Skip to content
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.

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

Support HTTP headers with CORS-method #2957

Merged

Conversation

MagMar94
Copy link
Contributor

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.

A separate file makes it easier to test. Added unit tests to the cors-method.
@MagMar94 MagMar94 force-pushed the feature/support-http-headers-with-cors branch 3 times, most recently from d791a94 to 351433b Compare October 25, 2022 17:41
@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

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.

package.json Outdated Show resolved Hide resolved
tests/unit/functions/server_functions_spec.js Outdated Show resolved Hide resolved
@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.

@MagMar94 MagMar94 force-pushed the feature/support-http-headers-with-cors branch from 351433b to 5c044fc Compare October 25, 2022 19:36
@codecov-commenter
Copy link

codecov-commenter commented Oct 25, 2022

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 MagMar94 force-pushed the feature/support-http-headers-with-cors branch from 5c044fc to 8588044 Compare October 25, 2022 19:42
@MagMar94
Copy link
Contributor Author

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 force-pushed the feature/support-http-headers-with-cors branch 2 times, most recently from 43dcfc1 to f0319c2 Compare October 25, 2022 19:56
js/server_functions.js Outdated Show resolved Hide resolved
js/server_functions.js Outdated Show resolved Hide resolved
js/server_functions.js Outdated Show resolved Hide resolved
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

@MagMar94 MagMar94 force-pushed the feature/support-http-headers-with-cors branch from f0319c2 to e38c61f Compare October 29, 2022 17:50
This is required for some weather-providers, and will probably be useful for other services.
@MagMar94 MagMar94 force-pushed the feature/support-http-headers-with-cors branch from e38c61f to ccc6732 Compare October 29, 2022 17:55
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.

js/server_functions.js Outdated Show resolved Hide resolved
js/server_functions.js Outdated Show resolved Hide resolved
tests/unit/functions/server_functions_spec.js Outdated Show resolved Hide resolved
@MagMar94 MagMar94 requested a review from rejas October 30, 2022 15:10
@MagMar94 MagMar94 force-pushed the feature/support-http-headers-with-cors branch from ccc6732 to 68e321b Compare October 30, 2022 15:31
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants