jspm / generator

JSPM Import Map Generator
Apache License 2.0
160 stars 20 forks source link

chore: pin down semantics of freeze with a bunch of test cases #276

Closed Bubblyworld closed 1 year ago

Bubblyworld commented 1 year ago

Added a comment in test/api/freeze.test.js specifying the intended semantics of the freeze flag and a bunch of tests pinning down all the cases:

/**
 * This test pins down the semantics of the "freeze" option on the generator.
 *
 * When enabled, the entire input map is treated as a strict lockfile, meaning
 * no existing versions of any dependency will be changed by the generator. If
 * there's a secondary lock for "react", for instance, then even a primary
 * install will use that lock rather than latest. Freeze allows new dependencies
 * to be added, however, if they have no existing locks.
 *
 * When freeze is combined with "resolutions", the custom resolutions always
 * always take precedence over any of the freeze behaviour.
 *
 * When freeze is combined with "latest", the latest flag takes precedence and
 * all locks are upgraded to the latest compatible versions.
 */
guybedford commented 1 year ago

If there's a secondary lock for "react", for instance, then even a primary install will use that lock rather than latest.

Perhaps we should change this case specifically? One rule I've found useful here traditionally has been that if the existing version can satisfy the primary range then use it, otherwise allow the primary to diverge. Also if it is in "peerDependencies" that usually means there must strictly only be a singular version of the package and it should not split.

When freeze is combined with "resolutions", the custom resolutions always always take precedence over any of the freeze behaviour.

Great

When freeze is combined with "latest", the latest flag takes precedence and all locks are upgraded to the latest compatible versions.

Can we just say that freeze and latest are incompatible and that if using both, we throw an error instead?

Bubblyworld commented 1 year ago

Perhaps we should change this case specifically?

Makes sense, this still respects the main constraint which is freeze doesn't change existing locks.

Can we just say that freeze and latest are incompatible and that if using both, we throw an error instead?

Yeah, best we don't silently ignore the freeze flag.

guybedford commented 1 year ago

The failure here now seems directly related to the freeze test itself.

Bubblyworld commented 1 year ago

...freeze test is fixed and works locally, but there's another build failure =( I'm a sad panda.