Skip to content

Consolidate dependencies and remove duplicates#4883

Merged
dcousens merged 1 commit intoJedWatson:masterfrom
jgeurts:consolidate-deps
Oct 19, 2022
Merged

Consolidate dependencies and remove duplicates#4883
dcousens merged 1 commit intoJedWatson:masterfrom
jgeurts:consolidate-deps

Conversation

@jgeurts
Copy link
Contributor

@jgeurts jgeurts commented Oct 28, 2021

Hi! Hope this PR finds you well. I'm not sure how useful this is since it seems that preconstruct does similar at build time.

  • Move most dependencies to be devDependencies
  • Reduce duplication of dependencies

I ran through all of the scripts in package.json and things appear to work as they did prior to this PR.

@changeset-bot
Copy link

changeset-bot bot commented Oct 28, 2021

⚠️ No Changeset found

Latest commit: 2060f64

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 28, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2060f64:

Sandbox Source
react-codesandboxer-example Configuration

Copy link
Collaborator

@Methuselah96 Methuselah96 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Can you keep all the dependencies for the root package and docs package in dependencies instead of splitting it into devDependencies and dependencies? For those packages, it doesn't really matter whether it's a dependency or dev dependency since it will not ever get installed by another package.

@jgeurts
Copy link
Contributor Author

jgeurts commented Nov 9, 2021

I'm curious how come you prefer to put everything in dependencies and not lean on devDependencies?

@Methuselah96
Copy link
Collaborator

Methuselah96 commented Nov 11, 2021

@jgeurts It's purely a stylistic choice (imo) that Jed said he preferred in our maintainers meeting when we looked at this PR. Since those packages are never used outside of this repository there is no functional difference between them being dependencies or devDependencies so might as well just stick them all in dependencies.

(Personally I prefer the semantic difference between dependencies and devDependencies even for apps where it doesn't make a functional difference since it still designates which dependencies are packaged by Webpack and used at runtime, but to each his own and it's Jed's project.)

@Methuselah96
Copy link
Collaborator

Methuselah96 commented Nov 11, 2021

Or I guess another way of thinking about it is that the docs and root packages are primarily Node packages and the "dev dependencies" are in fact a runtime dependency since the main function of those packages is to start a Webpack build.

@jgeurts
Copy link
Contributor Author

jgeurts commented Nov 11, 2021

@jgeurts It's purely a stylistic choice (imo) that Jed said he preferred in our maintainers meeting when we looked at this PR. Since those packages are never used outside of this repository there is no functional difference between them being dependencies or devDependencies so might as well just stick them all in dependencies.

Cool, thanks for the insight!

@dcousens dcousens force-pushed the consolidate-deps branch 3 times, most recently from 55948c5 to cbd408f Compare October 19, 2022 05:15
* Move most dependencies to be devDependencies
* Reduce duplication of dependencies
@dcousens dcousens enabled auto-merge October 19, 2022 05:33
@dcousens dcousens merged commit 46f9263 into JedWatson:master Oct 19, 2022
@dcousens
Copy link
Collaborator

dcousens commented Oct 19, 2022

This is always going to be an arbitrary balance, but I think this is helpful enough as is that we can refine anything else in future pull requests.

@jgeurts jgeurts deleted the consolidate-deps branch October 19, 2022 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants