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

Docs: Clarify how to update a dependency for existing packages #16923

Merged
merged 1 commit into from
Aug 8, 2019

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Aug 6, 2019

Description

This is a follow-up for #16876 where I added some hints on how to add a new dependency to one of the WordPress packages. This time I add more details for removing and updating dependencies.

It was triggered by the discussion in #16875. I tested it myself when upgrading ESLint and its plugins to the latest version in #16921.

Preview new version of docs here:
https://github.com/WordPress/gutenberg/blob/db23b8c4abeb6c8711060e78cfbab1648ad7f27a/packages/README.md#managing-dependencies

@gziolo gziolo added [Type] Developer Documentation Documentation for developers npm Packages Related to npm packages labels Aug 6, 2019
@gziolo gziolo self-assigned this Aug 6, 2019
@ellatrix
Copy link
Member

ellatrix commented Aug 6, 2019

My biggest problem was that I assumed npm install generates a good lock file based on all package.json files. This turned out to be false. It would be col if somehow npm install resolves everything behind the scenes.

@gziolo
Copy link
Member Author

gziolo commented Aug 6, 2019

My biggest problem was that I assumed npm install generates a good lock file based on all package.json files. This turned out to be false. It would be col if somehow npm install resolves everything behind the scenes.

Yes, the issue is that for some reasons npm install puts those dependencies inside node_modules folder of the package, but we want to hoist them to the root node_modules folder.

@gziolo gziolo added this to the Gutenberg 6.3 milestone Aug 6, 2019
@dsifford
Copy link
Contributor

dsifford commented Aug 6, 2019

but we want to hoist them to the root node_modules folder.

Doesn't lerna bootstrap --hoist do that?

@gziolo
Copy link
Member Author

gziolo commented Aug 6, 2019

but we want to hoist them to the root node_modules folder.

Doesn't lerna bootstrap --hoist do that?

Did you try or do you guess?

@dsifford
Copy link
Contributor

dsifford commented Aug 6, 2019

but we want to hoist them to the root node_modules folder.

Doesn't lerna bootstrap --hoist do that?

Did you try or do you guess?

Both! Recall that's what I did first before talking to you originally about this.

@gziolo
Copy link
Member Author

gziolo commented Aug 7, 2019

Both! Recall that's what I did first before talking to you originally about this.

Can you advice how the process should look like with lerna bootstrap? Is it significantly better than what I proposed?

@dsifford
Copy link
Contributor

dsifford commented Aug 7, 2019

I believe it would prevent having to remove and re-add packages when attempting to update a nested dependency. You should in theory just have to update the package.json and bootstrap.

There's not a ton different I suppose than to what you're describing so it might just be easier for everyone to stick with what you wrote since everybody knows the npm install command already.

@gziolo gziolo merged commit 4665d70 into master Aug 8, 2019
@gziolo gziolo deleted the update/docs-updating-package-dependency branch August 8, 2019 05:19
@gziolo
Copy link
Member Author

gziolo commented Aug 8, 2019

I believe it would prevent having to remove and re-add packages when attempting to update a nested dependency. You should in theory just have to update the package.json and bootstrap.

It seems to be a bit easier when I read it :) I merged this PR to have any hints present but I will give it a go and see if I can further refine docs. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm Packages Related to npm packages [Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants