search

ember-cli / ember-try

An ember-cli addon to test against multiple npm dependencies, such as ember and ember-data.
MIT License
179 stars 58 forks source link

Update and test pnpm behavior so that we simulate a new user install #958

Closed NullVoxPopuli closed 1 year ago

NullVoxPopuli commented 1 year ago

Based on discussion here: https://discord.com/channels/480462759797063690/480501885837770763/1122582845873983529

codecov[bot] commented 1 year ago

Codecov Report

Merging #958 (2a9a5ef) into master (c590272) will decrease coverage by 0.17%. The diff coverage is 75.00%.

:exclamation: Current head 2a9a5ef differs from pull request most recent head 27c856a. Consider uploading reports for the commit 27c856a to get more accurate results

@@            Coverage Diff             @@
##           master     #958      +/-   ##
==========================================
- Coverage   94.54%   94.38%   -0.17%     
==========================================
  Files          17       17              
  Lines         550      552       +2     
==========================================
+ Hits          520      521       +1     
- Misses         30       31       +1
Impacted Files Coverage Δ
lib/utils/dependency-manager-adapter-factory.js 91.30% <ø> (ø)
lib/dependency-manager-adapters/pnpm.js 89.41% <75.00%> (-0.95%) :arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

kategengler commented 1 year ago

I wonder if we should make this new behavior have an opt-in or out?

Users have the capability to override --ignore-lockfile for the other managers if they do want to test with a lockfile.

NullVoxPopuli commented 1 year ago

that's a good point :thinking:

are you thinking it'd be like, a top-level config option like usePnpm?

like, maybe:

{
  usePnpm: true,
  adapterOptions: { // specific to each adapter, with their own defaults
    keepLockfile: true, // default false
  }

}

idk

kategengler commented 1 year ago

@NullVoxPopuli I think that's a bit too close to buildManagerOptions. I think it should be opt-out, same as the other managers (though theirs are through flags). I don't really love adding more top-level config, but maybe we should here: floatDependencies: true. We could make it work for the other managers too, via their command line flags, though it would interact unfortunately with buildManagerOptions and npmOptions.

Alternatively, I think there's some precedent in the repo for making our own flags that can be supplied in npmOptions or buildManagerOptions that we remove before passing them along.

NullVoxPopuli commented 1 year ago

Going to do a separate PR that uses this: image