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

feat(medusa): Allow regexp in CORS configuration #1935

Merged
merged 19 commits into from
Dec 12, 2022

Conversation

adrien2p
Copy link
Member

@adrien2p adrien2p commented Jul 28, 2022

What
In some cases, a users that works with a bunch of subdomain would like to be able to configure the cors using a regexp to allow all or a sub set of his sub domains
https://discord.com/channels/876835651130097704/877195433649258536/1002330864216195112

Info
Cors middleware impl authorised it:
https://github.com/expressjs/cors/blob/f038e7722838fd83935674aa8c5bf452766741fb/lib/index.js#L19

@adrien2p adrien2p self-assigned this Jul 28, 2022
@adrien2p adrien2p requested a review from a team as a code owner July 28, 2022 22:27
@changeset-bot
Copy link

changeset-bot bot commented Jul 28, 2022

🦋 Changeset detected

Latest commit: 535705e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@medusajs/medusa Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@adrien2p adrien2p force-pushed the feat/cors-allowed-regexp branch 2 times, most recently from b7309e9 to 3633933 Compare July 28, 2022 22:33
@adrien2p adrien2p changed the title feat(medusa): Allowed regexp in cors feat(medusa): Allow regexp in cors Jul 28, 2022
@olivermrbl olivermrbl changed the title feat(medusa): Allow regexp in cors feat(medusa): Allow regexp in CORS configuration Aug 8, 2022
@olivermrbl olivermrbl force-pushed the feat/cors-allowed-regexp branch 2 times, most recently from ebaadbc to 57f1255 Compare August 8, 2022 14:52
@adrien2p adrien2p force-pushed the feat/cors-allowed-regexp branch 2 times, most recently from db92c50 to 76d59e4 Compare October 11, 2022 07:33
@adrien2p
Copy link
Member Author

adrien2p commented Nov 7, 2022

@olivermrbl do you think that pr could be merge at some point?

@olivermrbl
Copy link
Contributor

@adrien2p Yes, I will give this a review tomorrow :)

@olivermrbl olivermrbl mentioned this pull request Nov 10, 2022
@pKorsholm pKorsholm requested a review from olivermrbl November 15, 2022 04:26
@olivermrbl
Copy link
Contributor

olivermrbl commented Nov 15, 2022

I am attempting to configure ADMIN_CORS with a regex, but I can't seem to make it work. Am I doing something wrong here?

const ADMIN_CORS = `/http\:\/\/localhost\:700\d`

This should allow requests from origins http://localhost:7000, http://localhost:7001, etc.

@adrien2p
Copy link
Member Author

adrien2p commented Nov 15, 2022

@olivermrbl I ve open a pr in the CORS repo as I can't seam to be able to find the culprit expressjs/cors#292

It is also possible that the changeset need to be updated

@adrien2p
Copy link
Member Author

adrien2p commented Nov 16, 2022

It is now working properly, just need to double the \ to avoid them to be escaped
/http:\\/\\/localhost:700\\d+$/

@carlos-r-l-rodrigues
Copy link
Contributor

That's a great functionality.

Question: I believe we will increase quite a lot the size of the plugins if they now depend on medusa core. Should we think about separating the "utils" to a new package?
ps: considering that some people are not using package managers that reuse installed node_modules

@adrien2p
Copy link
Member Author

adrien2p commented Nov 18, 2022

That's a great functionality.

Question: I believe we will increase quite a lot the size of the plugins if they now depend on medusa core. Should we think about separating the "utils" to a new package? ps: considering that some people are not using package managers that reuse installed node_modules

I see what you mean, but plugins already depends on the core as they can extend strategies, interfaces, services, and also are already using some utilities. In that case I am afraid it would mean to separate all of those in different packages. Generally speaking, since the plugins have no dependency to the core, but only as a peer dependency, I don't think it is a problem since the core will then not be installed by the plugin. they are normally used in the server and the server install medusa core already wdyt? let me know if I have miss understood something

@carlos-r-l-rodrigues
Copy link
Contributor

carlos-r-l-rodrigues commented Nov 18, 2022

Sorry, disregard my comment. I see it's listed as a peerDependencies. Even npm will reuse medusa-code if that's is installed.

@adrien2p adrien2p force-pushed the feat/cors-allowed-regexp branch from 26c7c96 to 37d8ce2 Compare November 25, 2022 08:38
@adrien2p
Copy link
Member Author

@olivermrbl finally all green

@olivermrbl
Copy link
Contributor

Think we should add a guide for this in our documentation. Wdyt? @adrien2p

cc. @shahednasser

@adrien2p
Copy link
Member Author

adrien2p commented Nov 28, 2022

Yes I agree, we can take the examples we've put in comments of that pr? And we should mention the usage of the utilities in custom routes

@shahednasser
Copy link
Member

I'll add an issue for it in Linear 👍🏻

@adrien2p adrien2p force-pushed the feat/cors-allowed-regexp branch from 34c8c95 to 532be0e Compare November 28, 2022 11:19
@srindom
Copy link
Collaborator

srindom commented Dec 12, 2022

@olivermrbl, @adrien2p - can we merge this?

@olivermrbl
Copy link
Contributor

Yes, we were just awaiting a green pipeline. After resolving the conflict, we should be good 💪

@adrien2p
Copy link
Member Author

@srindom yes sure, i ve tested it and was waiting for someone else to test as well

Copy link
Contributor

@olivermrbl olivermrbl left a comment

Choose a reason for hiding this comment

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

LGTM

@olivermrbl olivermrbl merged commit b5d6682 into develop Dec 12, 2022
@olivermrbl olivermrbl deleted the feat/cors-allowed-regexp branch December 12, 2022 18:24
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.

5 participants