-
Notifications
You must be signed in to change notification settings - Fork 78
fix(node.js) Update documentation related to devDependencies pruning #3326
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
base: master
Are you sure you want to change the base?
Conversation
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.
praise: thanks a lot for trying to clarify this. It's been a subject for a long time.
I've added a few suggestions, which are mostly nitpicks.
I'm wondering if this is the behavior we really want 🤔
@EtienneM: WDYT?
Maybe we could also add your table @aurelien-reeves-scalingo? It would make things crystal-clear, wouldn't it?
Finally, we also have a PR here, waiting for approval. I don't think it would have consequences on yours, but could please double-check? 🙏
Thanks a lot <3
Co-authored-by: François <[email protected]>
Co-authored-by: François <[email protected]>
Thanks for pointing me out to that PR. It makes me realized that I don't understand how the yarn pruning is working. My tests tend to show that the pruning of dependencies with yarn is working as expected, but from reading the code (on master), I have no idea how it actually works. In the end, there is no consequences, no impact from one PR to another, that's for sure. |
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.
I don't understand the "cache" column in your table.
Otherwise I agree with François that the table could be a good addition to our documentation to clarify how the buildpack behave with Yarn v1, Yarn v2 and npm.
| Then you can use this CLI command pattern: `scalingo --app my-app env-set variable=value` | ||
|
|
||
| Example for npm : `scalingo --app my-app env-set NPM_CONFIG_PRODUCTION=true` | ||
| If you need access to packages declared under `devDependencies` at runtime - or in a different buildpack - then you'll need to use yarn v2+ and set `YARN2_SKIP_PRUNING=true`. |
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.
I'm not keen on this change. I don't think we want to force customers to use Yarn if they need to access dependencies declared in devDependencies.
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.
There is no other alternative right now
| most common error messages. | ||
|
|
||
| ## devDependencies Also Contain the Build Dependencies {#dep} | ||
| ## devDependencies Also Contain Some Dependencies Required at Startup or Runtime {#dep} |
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.
I think this section should highlight the type of errors a customer may face, and link to languages/nodejs/2000-01-01-start#devdependencies-installation to explain how to circumvent them.
Documentation regarding node.js devDependencies pruning is wrong.
I did some tests to make sure of actual behavior: it leads into the following:
It appears that, at the moment, the only way to keep dev dependencies as part of the image is to use yarn v2+ with
YARN2_SKIP_PRUNING