feat: match npm semantics exactly for installs and updates #296
Conversation
Bubblyworld
force-pushed
the
guy-npm_semantics
branch
from
1863e31
to
0a59554
Compare
Bubblyworld
force-pushed
the
guy-npm_semantics
branch
from
3321d26
to
f5347e8
Compare
Bubblyworld
force-pushed
the
guy-npm_semantics
branch
from
2c8b25e
to
4830298
Compare
Bubblyworld
force-pushed
the
guy-npm_semantics
branch
from
4830298
to
02924eb
Compare
|
So I've replaced all of the free/latest/mode options and replaced them with a single mode parameter that controls existing locks:
|
| @@ -345,7 +345,7 @@ export default class TraceMap { | |||
| dynamics.map(async ([specifier, parent]) => { | |||
| await this.visit( | |||
| specifier, | |||
| { visitor, installOpts: { mode: "existing" }, toplevel: false }, | |||
| { visitor, installMode: "freeze", toplevel: false }, | |||
There was a problem hiding this comment.
This internal unification is really nice to see!!
| async install( | ||
| install?: string | Install | (string | Install)[] | ||
| ): Promise<void | { staticDeps: string[]; dynamicDeps: string[] }> { | ||
| if (arguments.length > 1) | ||
| throw new JspmError( | ||
| "Install takes no arguments, a single install target, or a list of install targets." | ||
| ); | ||
| return this._install(install); | ||
| } | ||
|
|
||
| private async _install( |
There was a problem hiding this comment.
Not sure I fully understand the reason for this private _install function?
There was a problem hiding this comment.
Depending on whether install is given an argument or not, two different modes are used:
- No argument - all top-level locks installed in
defaultmode, i.e. keep all in-range locks. - Arguments - all arguments installed in
latest-primariesmode, i.e. bump those arguments to latest but keep in-range secondaries.
This just lets us handle those cases without exposing the mode argument in the public API.
There was a problem hiding this comment.
Ah, I missed that we were keeping mode part of the private API. Should we perhaps just expose it though with defaults? Is there a risk in doing that?
|
|
||
| // If there are no arguments, then we reinstall all the top-level locks: | ||
| if (!install) { | ||
| if (install === null || install === undefined) { |
There was a problem hiding this comment.
This was previously handled via generator.reinstall. I believe this change means that reinstall is now deprecated?
There was a problem hiding this comment.
The reinstall() method is equivalent to freeze installing all the top-level locks (or equivalently doing an argumentless link, since link uses freeze by default). So I think it can be deprecated now in favour of either of those, yeah.
There was a problem hiding this comment.
Ok, let's do that then, either with this PR or via a follow-up after it lands.
Currently some tests are failing due to the new behaviour, I'm still working
my way through them. There's a script you can run at
test/npm-compatibility/that will test the behaviour of npm for updates/installs in a bunch of different
scenarios, and tests that make sure the behaviour of the generator is aligned.
npm install <pkg>bumps the version of<pkg>to latest, and only bumpstransitive dependencies if they're out of range.
npm update <pkg>has exactly the same behaviour as 1 in all tested cases.npm installbumps the versions of anything that is out of range, but keepsversions of in-range primaries and secondaries.
npm updatebumps the versions of everthing to latest.The changes to
latestthat I've made:latestandfreezegenerator options have both been deprecated infavour of a
ResolutionOptionsobject that you can optionally pass intoinstall/link/update.ResolutionsOptionscontainsmode,latestPrimaries,latestSecondariesand
freezefields. I had to split into the two cases for latest to makesure that
npm install <pkg>doesn't bump secondaries, but does bumpprimaries.
latest/freezeresolution options are set, the defaultis to take existing locks whenever they are in range.