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): Preserve node flags in develop command #1860

Merged
merged 8 commits into from
Jun 14, 2023

Conversation

liamjcooper
Copy link
Contributor

@liamjcooper liamjcooper commented Jul 16, 2022

What

I have added the node --preserve-symlinks flag to the child process that gets created when running the Medusa develop command.

Why

To help streamline the process of creating a new plugin - as I found this was a noticeable stumbling block for me. Otherwise, it is not possible to run a Medusa server with the develop command while testing a plugin locally.

How

Converted the "cross-spawn" package usage to Node's standard lib "child_process" fork method. This provides the child process with sensible defaults (current directory, current environment, standard i/o...) and passes node flags from its parent, as well as adding the --preserve-symlinks as one of the default flags to Node.

Testing

I tested this by building the medusa package and copying the newly built develop.js file to my node_modules/@medusajs/medusa/dist/commands directory in my Medusa project. Certainly not the best workflow, but I was finding that when I linked a local version of the medusa package to my Medusa project the symlinks for my plugin would still get resolved whether or not the --preserve-symlinks flag was passed... probably a knowledge issue with dependencies that I couldn't get my head around.

This aims to fix issue #1859.

Also, with this PR, perhaps cross-spawn is no longer needed as a dependency?

@changeset-bot
Copy link

changeset-bot bot commented Jul 16, 2022

🦋 Changeset detected

Latest commit: 440a9a8

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

This PR includes changesets to release 11 packages
Name Type
@medusajs/medusa Patch
@medusajs/admin-ui Patch
@medusajs/admin Patch
@medusajs/medusa-js Patch
medusa-payment-paypal Patch
medusa-payment-stripe Patch
medusa-plugin-mailchimp Patch
medusa-plugin-restock-notification Patch
medusa-react Patch
@medusajs/medusa-oas-cli Patch
@medusajs/oas-github-ci 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

Copy link
Member

@adrien2p adrien2p left a comment

Choose a reason for hiding this comment

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

Good 🎉 few suggestions

@liamjcooper liamjcooper changed the title Preserve symlinks in develop command Preserve node flags in develop command Jul 17, 2022
@liamjcooper liamjcooper marked this pull request as ready for review July 18, 2022 13:22
@liamjcooper liamjcooper requested a review from a team as a code owner July 18, 2022 13:22
@adrien2p adrien2p requested a review from pKorsholm July 22, 2022 08:27
@liamjcooper
Copy link
Contributor Author

liamjcooper commented Jul 22, 2022

I think it's now possible to remove the cross-spawn dependency from the project now that we fork the new process with Nodes child_process from stdlib. I did a codebase search and couldn't find more references, but interested in your opinion.

@liamjcooper liamjcooper requested a review from a team as a code owner July 22, 2022 12:33
@liamjcooper liamjcooper force-pushed the fix/symlinks-on-dev-command branch 5 times, most recently from 891906a to 834a0c0 Compare July 22, 2022 13:39
@adrien2p
Copy link
Member

@srindom do you think that this could get a review at some point. It would avoid a user to have to remove the node modules from the peers in the plugin as explain in the documentation and instead use the preserve flag. wdyt?

@olivermrbl olivermrbl changed the title Preserve node flags in develop command feat(medusa): Preserve node flags in develop command Jun 14, 2023
@vercel
Copy link

vercel bot commented Jun 14, 2023

@olivermrbl is attempting to deploy a commit to the medusajs Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Jun 14, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
medusa-docs ⬜️ Ignored (Inspect) Jun 14, 2023 11:38am

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 - appreciate the contribution 🚢

@adrien2p okay to merge?

Copy link
Member

@adrien2p adrien2p left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution and sorry for the delay ❤️

@olivermrbl olivermrbl merged commit ca477c8 into medusajs:develop Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants