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

Upgrade CI / re-roll lockfile #455

Merged
merged 15 commits into from
Sep 25, 2023

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented May 4, 2023

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 NullVoxPopuli changed the title @ember/string is required for ember-beta and ember-canary to go green Get CI green May 4, 2023
@NullVoxPopuli NullVoxPopuli marked this pull request as draft May 4, 2023 13:48
@NullVoxPopuli
Copy link
Contributor Author

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

@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review May 4, 2023 23:27
@rwjblue
Copy link
Member

rwjblue commented May 5, 2023

FWIW the ember-beta and ember-canary issue in CI is unrelated to your changes. It's basically emberjs/ember.js#20283 (but happening again) due to emberjs/ember.js@784378f.

@NullVoxPopuli
Copy link
Contributor Author

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.

@NullVoxPopuli NullVoxPopuli changed the title Get CI green Get CI green + Upgrade CI / re-roll lockfile May 5, 2023
@NullVoxPopuli NullVoxPopuli changed the title Get CI green + Upgrade CI / re-roll lockfile Upgrade CI / re-roll lockfile May 5, 2023
@scalvert
Copy link
Collaborator

scalvert commented Sep 8, 2023

@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
Copy link
Contributor Author

NullVoxPopuli commented Sep 25, 2023

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

Due to:
  message: |+
    Died on test emberjs#1: Cannot read properties of undefined (reading 'allowCachingPerBundle')
        at Object.<anonymous> (/home/runner/work/ember-test-waiters/ember-test-waiters/node-tests/force-highlander-addon-test.js:176:5)

We don't need to be testing that both `ember-test-waiters` and
`@ember/test-waiters` exist as a part of a build-time transform -- we
can published two packages.
@scalvert scalvert merged commit 579c090 into emberjs:master Sep 25, 2023
14 checks passed
scalvert added a commit that referenced this pull request Oct 2, 2023
@scalvert
Copy link
Collaborator

scalvert commented Oct 2, 2023

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

@NullVoxPopuli
Copy link
Contributor Author

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
Copy link
Contributor Author

NullVoxPopuli commented Oct 2, 2023

The file is also removed here: #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
Copy link
Collaborator

scalvert commented Oct 2, 2023

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
Copy link
Contributor Author

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,

  • test throw an error when that happens, and ask the consumer / app to fix their dependencies (like how qunit does).
  • or support duplicates by changing where the module-level state is kept today to be more stable and elsewhere

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
Copy link
Contributor Author

I've updated the plan laid out here: #458

@NullVoxPopuli NullVoxPopuli deleted the fix-ember-beta-canary branch October 3, 2023 01:42
@github-actions github-actions bot mentioned this pull request Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants