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

fix(cli): prevent production install on add cmd #8870

Merged
merged 2 commits into from
Oct 20, 2023

Conversation

xstevenyung
Copy link
Contributor

Changes

  • force NODE_ENV=development on installation command run when using astro add command to prevent installation to remove devDependencies

Testing

no real way to test

Docs

no need

fix #8037

@changeset-bot
Copy link

changeset-bot bot commented Oct 19, 2023

🦋 Changeset detected

Latest commit: d5afe62

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

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

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Oct 19, 2023
@@ -700,7 +700,7 @@ async function tryToInstallIntegrations({
...inheritedFlags,
...installCommand.dependencies,
],
{ cwd }
{ cwd, env: { NODE_ENV: 'development' } }
Copy link
Member

Choose a reason for hiding this comment

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

Interesting! We actually set the NODE_ENV here so the config file is loaded correctly:

process.env.NODE_ENV = cmd === 'dev' ? 'development' : 'production';

On this line, could we set NODE_ENV: undefined instead to reset the value? Also would be great to add an additional comment explaining why we need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the reset seems to work with undefined.
i also added a comment to explain why we are doing this 👍

@xstevenyung xstevenyung force-pushed the fix/remove-dev-deps-on-add-cmd branch from 5c1ac7c to 087e8ce Compare October 19, 2023 16:42
@jacobdalamb
Copy link
Contributor

Can’t wait for this PR to be merged

@ematipico
Copy link
Member

@xstevenyung can you add a changeset via pnpm changeset?

@xstevenyung xstevenyung force-pushed the fix/remove-dev-deps-on-add-cmd branch from 087e8ce to a1669c1 Compare October 20, 2023 12:11
@github-actions github-actions bot added the pr: docs A PR that includes documentation for review label Oct 20, 2023
@xstevenyung
Copy link
Contributor Author

added the changeset as a minor bump

@bluwy bluwy merged commit 5ea6ee0 into withastro:main Oct 20, 2023
4 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

astro add causes dev dependencies to be removed from node_modules
4 participants