-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Allow dynamic import() requests from any origin on any device. #9954
Allow dynamic import() requests from any origin on any device. #9954
Conversation
@call-a3: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, just a few notes!
packages/dynamic-import/server.js
Outdated
@@ -66,7 +66,11 @@ function middleware(request, response) { | |||
response.setHeader("Access-Control-Allow-Origin", "*"); | |||
|
|||
if (request.method === "OPTIONS") { | |||
response.setHeader("Access-Control-Allow-Headers", "*"); | |||
if (request.headers["access-control-request-headers"] !== undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to worry about different possible capitalizations when checking for this header?
Also we typically test typeof value === "undefined"
rather than relying on the undefined
identifier, just because undefined
is mutable in some environments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to worry about capitalization, all keys of headers
are lower-cased by node.
packages/dynamic-import/server.js
Outdated
@@ -66,7 +66,11 @@ function middleware(request, response) { | |||
response.setHeader("Access-Control-Allow-Origin", "*"); | |||
|
|||
if (request.method === "OPTIONS") { | |||
response.setHeader("Access-Control-Allow-Headers", "*"); | |||
if (request.headers["access-control-request-headers"] !== undefined) { | |||
response.setHeader("Access-Control-Allow-Headers", request.headers["access-control-request-headers"]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Especially if we end up having to check for multiple variations of this header, it would be good to store the value in a variable that we could reuse down here.
A tweak to the change introduced in c4b5707 to fix #9952.
This will allow clients that don't support the * value in
Access-Control-Allow-Headers
,but do specify the
Access-Control-Request-Headers
(such as electron 2.0.2) to use dynamic import.