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

Remove code for pre-ember-v4 #1382

Merged
merged 10 commits into from
May 31, 2023

Conversation

NullVoxPopuli
Copy link
Collaborator

Ember v4 is a requirement for the next release of @ember/test-helpers, so we can drop some code branches.

@NullVoxPopuli NullVoxPopuli force-pushed the remove-code-for-pre-ember-v4 branch from 3e22660 to da9ec25 Compare May 2, 2023 03:36
@NullVoxPopuli
Copy link
Collaborator Author

NullVoxPopuli commented May 2, 2023

Something seems wrong with @glimmer/manager :(
logic should be the same for >= 4.0 ember-source except, we switched from importSync to regular import? idk

@NullVoxPopuli NullVoxPopuli force-pushed the remove-code-for-pre-ember-v4 branch from da9ec25 to 0e68ac4 Compare May 31, 2023 00:30
@NullVoxPopuli NullVoxPopuli force-pushed the remove-code-for-pre-ember-v4 branch from 60c8f34 to c453f8c Compare May 31, 2023 00:53
@MelSumner MelSumner added the v3 label May 31, 2023
@MelSumner
Copy link
Contributor

Talked with @NullVoxPopuli and think this could also be good to have in a 3.x release.

@@ -129,55 +128,33 @@ export function render(
let OutletTemplate = lookupOutletTemplate(owner);
let ownerToRenderFrom = options?.owner || owner;

if (macroCondition(dependencySatisfies('ember-source', '<3.24.0'))) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code change is the removal of this if branch, keeping only the else

…ng @glimmer/manager brought in newer types -- this will be simplified in the next glimmer-vm release
…anager, since we can't add @glimmer/manager as a devDependency, becaues that triggers some correctness checking in webpack that (correctly, but fights against our weirdness) throws an error that we're importing from @glimmer/manager when it's a devDep instead of dep
// SAFETY: in more recent versions of @glimmer/manager,
// this throws an error when maybeComponent does not have
// an associated manager.
try {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that @glimmer/manager changed its behavior. This change absorbs the change in @glimmer/manager

@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review May 31, 2023 01:49
Copy link
Contributor

@MelSumner MelSumner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes seem correct to me, and I'll wait until tomorrow before merging in case anyone else wants to take a second look.

@MelSumner MelSumner merged commit cf7b621 into emberjs:master May 31, 2023
@NullVoxPopuli NullVoxPopuli deleted the remove-code-for-pre-ember-v4 branch May 31, 2023 14:49
@gitKrystan gitKrystan mentioned this pull request Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants