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

Bump dependencies #19

Merged
merged 2 commits into from
Sep 5, 2020
Merged

Bump dependencies #19

merged 2 commits into from
Sep 5, 2020

Conversation

aminya
Copy link
Member

@aminya aminya commented Sep 2, 2020

Description of the Change

This bumps the dependencies of apm.

Benefits

Using the last dependencies ensures the latest performance, functionality, and security improvements

Verification

All the tests pass in CI and locally.

@DeeDeeG
Copy link
Member

DeeDeeG commented Sep 2, 2020

If I recall correctly, a few dependency upgrades to latest do break the tests. colors is one of them. Also tar.Extract() needs to be replaced with tar.extract() or tar.x, and the tar option path: needs to be replaced with the option cwd:, as of the tar@3 major version bump. Also yargs@4 has breaking changes

Rather than fully updating them to latest, if there are any dependencies that npm also uses, we can sync with npm and the dependency will be deduped, so the built Atom is slightly lighter I guess. That was what I had in mind to do next for dependencies: sync them with npm.

We share the following (non-devDependencies) dependencies with npm:

[ colors: consider switching to --> "ansicolors": "~0.3.2" that npm uses ],
"hosted-git-info": "^2.8.8",
[ open: consider switching to --> "opener": "^1.5.1" that npm uses ],
"read": "~1.0.7",
"request": "^2.88.0",
"rimraf": "^2.7.1",
"semver": "^5.7.1",
"tar": "^4.4.13", !! major version out of sync with npm !!

All of these are already in sync with npm@6 at the moment, other than tar, see #20 for that. We use slightly different mosulde that do the same thing as npm. We could dedupe even more if we switched from colors to ansicolors, and if we switched form open to opener.

@DeeDeeG DeeDeeG mentioned this pull request Sep 2, 2020
@DeeDeeG
Copy link
Member

DeeDeeG commented Sep 2, 2020

I merged this fork's master into an old branch where I maximally upgraded everything possible (other than yargs, colors and async, which have some breaking changes that I didn't work out how to adapt to yet):

Take a look if you like. CI is passing. I'm not sure upgrading everything like that is actually a good idea, especially the devDependencies, but it seems like it should work.

master...DeeDeeG:update-dependencies-atomic

ncu -u -x coffee-script, grunt, grunt-contrib-coffee, grunt-shell, grunt-cli, node-gyp
npm install && npm audit fix
@aminya
Copy link
Member Author

aminya commented Sep 5, 2020

Rather than fully updating them to latest, if there are any dependencies that npm also uses, we can sync with npm and the dependency will be deduped, so the built Atom is slightly lighter I guess.

Apm runs in an external node process. There is no benefit in using the same versions as npm.

To make apm truly load faster, we should build it using Parcel to tree-shake the dependencies properly. This is in my TODOs. We can get rid of Grunt by using Parcel.

@aminya aminya marked this pull request as ready for review September 5, 2020 18:43
@aminya aminya merged commit ad26b01 into master Sep 5, 2020
@aminya
Copy link
Member Author

aminya commented Sep 5, 2020

The tests don't run correctly after this. I reverted this. We need to bump one by one until the issue shows itself.

@DeeDeeG
Copy link
Member

DeeDeeG commented Sep 5, 2020

Apm runs in an external node process. There is no benefit in using the same versions as npm.

I meant that since apm is a wrapper around npm, if we use the same dependencies that apm's internal copy of npm uses under the hood, we can dedupe.

The tests don't run correctly after this. I reverted this. We need to bump one by one until the issue shows itself.

You can investigate for yourself, but spoiler alert: I know the answer. colors, async and yargs have breaking changes such as a changed API. Everything but those three can be updated to the latest version, no problems. Or at least the tests will pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants