search

emberjs / ember-test-waiters

An Ember addon to allow @ember/test-helpers to manage asynchronous operations
MIT License
29 stars 15 forks source link

Upgrade CI / re-roll lockfile #455

Closed NullVoxPopuli closed 11 months ago

NullVoxPopuli commented 1 year ago

This PR is probably the biggest diff stats for least amount of actual diff I've had in a while 😅

Context, Plan, etc: https://gist.github.com/NullVoxPopuli/eafc7dad6547de5e730098498b829e1f

This PR should unblock everything else (the monorepening, test-apps extraction (got a few to make, given how we have @ember/test-waiters and ember-test-waiters and a couple ember-concurrency versions to support simultaneously))

Breaking: drop support for node 10, 12, 14, support only 16+ (which ... 16 is also almost EOL'd at this point)

NullVoxPopuli commented 1 year ago

not quite sure what is going on with ci just yet -- ran ember-beta locally and it was fully green :thinking:

rwjblue commented 1 year ago

FWIW the ember-beta and ember-canary issue in CI is unrelated to your changes. It's basically https://github.com/emberjs/ember.js/pull/20283 (but happening again) due to https://github.com/emberjs/ember.js/commit/784378f83f0921ee01681293062e74804adcfd02.

NullVoxPopuli commented 1 year ago

Thanks for the tip -- PR is green now -- but there are a lot of simultaneous changes -- I'll open a new PR with just what's needed.

scalvert commented 1 year ago

@NullVoxPopuli I'm just going to comment here once, but this applies to all the open PRs. Is there a specific order the PRs need to be merged in order to get all changes in for a new major?

NullVoxPopuli commented 11 months ago

Is there a specific order the PRs need to be merged in order to get all changes in for a new major?

No specific order, unless indicated -- this one should be first though, because last I checked CI was totally busted.

Only breaking change here is dropping node 10

scalvert commented 11 months ago

@NullVoxPopuli I missed the fact that you deleted the force-highlander-addon-test.js. What was the reason for removing it?

NullVoxPopuli commented 11 months ago

Maybe I misunderstood, but we don't want ember-test-waiters to exist, and only want @ember/test-waiters. And having both in one package is actually not possible in a v2 world, and we'd need to publish them as separate packages if we want to keep ember-test-waiters around (and re-export @ember/test-waiters, / force a peer).

I do not remember it being in this PR, but it does need to go.

NullVoxPopuli commented 11 months ago

The file is also removed here: https://github.com/emberjs/ember-test-waiters/pull/454 So it's possible that I made a mistake extracting work to this PR (the mistake being isolating the changes called out in the PR title/description, as the work itself is not a mistake)

scalvert commented 11 months ago

This has nothing to do with the dual packages, and everything to do with the fact that we want to guarantee there's only a single version of test waiters in the project. This is due to an internal cache that's being used - we need to ensure this isn't duplicated in multiple package versions, so we ensure that we only build the latest version of test waiters into the project.

The dual package naming has nothing to do with this.

NullVoxPopuli commented 11 months ago

Gotchya, well, as a v2 addon we don't have control over that explicitly. We cannot have that highlander code. In a traditional package environment, we can't change the build without build-time plugins -- we also don't need to, because, while bundlers can provide duplicates of the same package, we can either,

Most addons should also start migrating towards declaring @ember/test-waiters as a peerDependency to communicate to the package manager (and build tooling) that the app's version of the package is what should be resolved.

That said, when having a migration period, we obviously can't force everyone to upgrade to the latest v2-addon (soon) version of @ember/test-waiters without causing some trouble to consumers who haven't upgraded (as the waiter system today requires only one copy, due to module state).

So, I propose a common, known, yet not documented variable on globalThis, which, instead of defining data in just module-scope, like here: https://github.com/emberjs/ember-test-waiters/blob/master/addon/%40ember/test-waiters/build-waiter.ts#L8-L9 we access globalThis.WAITER_NAMES, so that multiple copies of @ember/test-waiters can use the same data.

However, this will require that all folks using the current major of @ember/test-waiters upgrade to a version that has the globalThis support, as well as the next major be able to access that data on globalThis.

NullVoxPopuli commented 11 months ago

I've updated the plan laid out here: https://github.com/emberjs/ember-test-waiters/issues/458