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

[core] Hoist duplicated dependencies #341

Merged

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Sep 22, 2020

Prevent the versions to be different, it forces to upgrade all at once.

@oliviertassinari oliviertassinari added the core Infrastructure work going on behind the scenes label Sep 22, 2020
@oliviertassinari oliviertassinari force-pushed the hoist-duplicated-dependencies branch from aa3d987 to 7de4778 Compare September 22, 2020 20:47
@oliviertassinari
Copy link
Member Author

@dtassone Should I rebase?

"rollup-plugin-sourcemaps": "^0.6.2",
"rollup-plugin-terser": "^5.3.0",
"rollup-plugin-typescript2": "^0.27.1"
"@material-ui/x-grid-modules": "^4.0.0-alpha.2"
Copy link
Member

Choose a reason for hiding this comment

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

this package does not exist anymore.

Copy link
Member Author

@oliviertassinari oliviertassinari Sep 25, 2020

Choose a reason for hiding this comment

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

Definitely, it's outdated:

Capture d’écran 2020-09-25 à 11 44 34

Are you happy with me pushing in the hoisting direction? #341 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

I think that yarn hoist the package automatically in workspaces or monorepos to root folder of the repo, unless pkgs contains different versions... So keeping the same version of pkgs is better as it avoids nested node_modules.
However removing the dev dependencies of each package is not that great as it makes it less obvious to understand what each package needs and what are their real dependencies. Also if we decide to move the package outside this repo then we would have to rebuild each package.json
Lastly in the root package.json we can easily end up with a bunch of dependencies that we won't know if they are being used by any of the nested packages.

Copy link
Member Author

@oliviertassinari oliviertassinari Sep 25, 2020

Choose a reason for hiding this comment

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

The arguments I see in favor:

  • Force to have a single version for the whole repository, no mess to manage between inconsistent versions
  • If we don't hoist http://dependabot.com/ will update each version for each package individually.

Copy link
Member Author

@oliviertassinari oliviertassinari Sep 25, 2020

Choose a reason for hiding this comment

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

less obvious to understand what each package needs and what are their real dependencies.

I think we can solve by having a single script command that all the packages use. It's what we do in the main repo and very likely what we want to do it here too so when we change something, we change it for all the packages at once. So I think it puts us in the right direction.

if we decide to move the package outside this repo then we would have to rebuild each package.json

It would only happen if we move it to mui-org/material-ui, where the same packages should be available with the latest version. So, I think that it's a good thing, it will help catch inconsistencies.

Lastly in the root package.json we can easily end up with a bunch of dependencies that we won't know if they are being used by any of the nested packages.

Probably true for any packages, there is only a tight link between the usage and its declaration. But yes, the fewer LOCs the simpler its to find dead dependencies. IMHO, the best answer to this is to be rigorous when deleting code and second guessing dependencies upgrade PRs.

@oliviertassinari oliviertassinari force-pushed the hoist-duplicated-dependencies branch from 3d65cf6 to f67cc4b Compare September 25, 2020 23:10
@oliviertassinari oliviertassinari merged commit 9a1f3a3 into mui:master Sep 28, 2020
@oliviertassinari oliviertassinari deleted the hoist-duplicated-dependencies branch September 28, 2020 11:03
dtassone pushed a commit to dtassone/material-ui-x that referenced this pull request Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants