Skip to content

[core] Hoist duplicated dependencies#341

Merged
oliviertassinari merged 1 commit into
mui:masterfrom
oliviertassinari:hoist-duplicated-dependencies
Sep 28, 2020
Merged

[core] Hoist duplicated dependencies#341
oliviertassinari merged 1 commit into
mui:masterfrom
oliviertassinari:hoist-duplicated-dependencies

Conversation

@oliviertassinari

@oliviertassinari oliviertassinari commented Sep 22, 2020

Copy link
Copy Markdown
Member

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

@oliviertassinari oliviertassinari added the internal Behind-the-scenes enhancement. Formerly called “core”. label Sep 22, 2020
@oliviertassinari
oliviertassinari force-pushed the hoist-duplicated-dependencies branch from aa3d987 to 7de4778 Compare September 22, 2020 20:47
Comment thread packages/grid/data-grid/tsconfig.build.json Outdated
@oliviertassinari

Copy link
Copy Markdown
Member Author

@dtassone Should I rebase?

Comment thread packages/grid/data-grid/package.json Outdated
"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
Copy Markdown
Contributor

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.

@oliviertassinari oliviertassinari Sep 25, 2020

Copy link
Copy Markdown
Member Author

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
Copy Markdown
Contributor

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.

@oliviertassinari oliviertassinari Sep 25, 2020

Copy link
Copy Markdown
Member Author

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:

@oliviertassinari oliviertassinari Sep 25, 2020

Copy link
Copy Markdown
Member Author

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

internal Behind-the-scenes enhancement. Formerly called “core”.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants