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

deps: add deduplicate script and apply yarn-deduplicate #683

Merged
merged 2 commits into from
Aug 25, 2020

Conversation

Bnaya
Copy link
Contributor

@Bnaya Bnaya commented Apr 14, 2020

Motivation:
https://github.com/atlassian/yarn-deduplicate#why-is-this-necessary

Benefits:
Smaller & less nested node_modules
less deps are loaded into node when running dev stuff

It's good to run this command after every deps changes

@agilgur5
Copy link
Collaborator

Huh, was looking at some of my recent PRs to change deps and did see duplicates -- but Yarn suggests that can't happen -- so I was confused. Guess Yarn has some bugs around this. Thanks for the add!

package.json Outdated Show resolved Hide resolved
@Bnaya
Copy link
Contributor Author

Bnaya commented Apr 14, 2020

Huh, was looking at some of my recent PRs to change deps and did see duplicates -- but Yarn suggests that can't happen -- so I was confused. Guess Yarn has some bugs around this. Thanks for the add!

That's behaviour of yarn is indeed cryptic, but not completely senseles.
There's more info and discussions around it on yarn issues and yarn-deduplicate docs

@Bnaya
Copy link
Contributor Author

Bnaya commented Apr 14, 2020

Worth noting:
I choose to use the "fewer" strategy and not "newer"

@Bnaya Bnaya requested a review from agilgur5 April 14, 2020 17:08
@agilgur5 agilgur5 added this to the v0.14.0 milestone Apr 20, 2020
@vercel
Copy link

vercel bot commented Aug 23, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/formium/tsdx/bi15xym0y
✅ Preview: https://tsdx-git-fork-bnaya-deps-dedup-yarn-lock.formium.vercel.app

@agilgur5 agilgur5 changed the title Deps/dedup yarn lock deps: add deduplicate script and apply yarn-deduplicate Aug 23, 2020
@agilgur5 agilgur5 added the scope: dependencies Pull requests that update a dependency file label Aug 23, 2020
Bnaya and others added 2 commits August 25, 2020 00:45
Motivation:
https://github.com/atlassian/yarn-deduplicate#why-is-this-necessary

Benefits:
Smaller & less nested node_modules
less deps are loaded into node when running dev stuff

It's good to run this command after every deps changes

Co-Authored-By: Anton Gilgur <agilgur5@gmail.com>
- `--list --fail` will make it error out with a list of the deps that
  need deduplication

- add yarn-deduplicate as a devDep since it's now used in a few places
  and would be good to cache install of it
  - ironically, adding it created a duplicate dep on semver, which made
    it fail precommit until I usd it to resolve that
Copy link
Collaborator

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

LGTM, rebased with new deps which doubled the amount of lines de-duped.

Added deduplicate to devDeps, to precommit, and to CI using the flags suggested. CI checks that it now works and the precommit check was verified running immediately after adding to devDeps, because that ended up adding a duplicate 😅

Thanks for the optimization @Bnaya

@agilgur5 agilgur5 merged commit 22133ce into jaredpalmer:master Aug 25, 2020
@agilgur5
Copy link
Collaborator

@all-contributors please add @Bnaya for bug, code

@allcontributors
Copy link
Contributor

@agilgur5

I've put up a pull request to add @Bnaya! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: optimization Performance, space, size, etc improvement scope: dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants