Skip to content

Support HTTP headers with CORS-method #2957

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
rejas merged 5 commits into from
Oct 30, 2022
Merged

Support HTTP headers with CORS-method #2957

rejas merged 5 commits into from
Oct 30, 2022

Conversation

@MagMar94
Copy link
Contributor

@MagMar94 MagMar94 commented Oct 25, 2022

Adds support for sending and receiving HTTP-headers when using the CORS-method.

This change is required for the Yr weather-provider introduced in #2948.

To make it easier to add unit tests I moved the server-functions into a separate file.

Moved server logic to separate file.
e27fd2e 
A separate file makes it easier to test. Added unit tests to the cors-method.
@MagMar94 MagMar94 changed the title Support HTTP headers with CORS-method Draft: Support HTTP headers with CORS-method Oct 25, 2022
@MagMar94
Copy link
Contributor Author

MagMar94 commented Oct 25, 2022

The tests fail because GitHub uses a different node-version. GitHub uses 16.17.1, whilst I used 19.0.0 locally. Will try to implement the tests to run on both.

KristjanESPERANTO
KristjanESPERANTO reviewed Oct 25, 2022
View reviewed changes
@khassel
Copy link
Collaborator

khassel commented Oct 25, 2022

The tests fail because GitHub uses a different node-version. GitHub uses 16.17.1, whilst I used 19.0.0 locally. Will try to implement the tests to run on both.

we are running the tests here on 3 node version, 14+16+18. Our fetch function uses the internal fetch of Node if Node version >=18 and node-fetch for older versions. May related to your problem with 14+16.

@codecov-commenter
Copy link

codecov-commenter commented Oct 25, 2022 •
edited
Loading

Codecov Report

Merging #2957 (f0319c2) into develop (7bd9443) will decrease coverage by 1.56%.
The diff coverage is 53.46%.

@@             Coverage Diff             @@
##           develop    #2957      +/-   ##
===========================================
- Coverage    65.28%   63.72%   -1.57%     
===========================================
  Files           14       15       +1     
  Lines          726      758      +32     
===========================================
+ Hits           474      483       +9     
- Misses         252      275      +23
Impacted Files Coverage Δ
js/server_functions.js 23.52% <23.52%> (ø)
js/app.js 69.53% <82.35%> (-0.72%) ⬇️
js/server.js 89.58% <84.84%> (+22.01%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@MagMar94
Copy link
Contributor Author

MagMar94 commented Oct 25, 2022

The tests fail because GitHub uses a different node-version. GitHub uses 16.17.1, whilst I used 19.0.0 locally. Will try to implement the tests to run on both.

we are running the tests here on 3 node version, 14+16+18. Our fetch function uses the internal fetch of Node if Node version >=18 and node-fetch for older versions. May related to your problem with 14+16.

Yep, working now 👍

@MagMar94 MagMar94 changed the title Draft: Support HTTP headers with CORS-method Support HTTP headers with CORS-method Oct 25, 2022
@MagMar94 MagMar94 mentioned this pull request Oct 25, 2022
Merged
rejas
rejas reviewed Oct 28, 2022
View reviewed changes
rejas
rejas reviewed Oct 28, 2022
View reviewed changes
rejas
rejas reviewed Oct 28, 2022
View reviewed changes
rejas
rejas reviewed Oct 28, 2022
View reviewed changes
rejas
rejas requested changes Oct 28, 2022
View reviewed changes
Copy link
Collaborator

@rejas rejas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some small things only

Added functionality for sending and recieving HTTP-headers.
1c8ea72 
This is required for some weather-providers, and will probably be useful for other services.
rejas
rejas requested changes Oct 30, 2022
View reviewed changes
Copy link
Collaborator

@rejas rejas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately some merge conflicts now after yesterays merges. See #2961 and #2952 for node18 and fetch disuccsion.

@MagMar94 MagMar94 requested a review from rejas October 30, 2022 15:10
@rejas rejas merged commit 4d47c08 into MagicMirrorOrg:develop Oct 30, 2022
@MagMar94 MagMar94 deleted the feature/support-http-headers-with-cors branch January 22, 2023 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@khassel khassel khassel left review comments

@KristjanESPERANTO KristjanESPERANTO KristjanESPERANTO left review comments

@rejas rejas Awaiting requested review from rejas

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@MagMar94 @khassel @codecov-commenter @rejas @KristjanESPERANTO