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

Drop @ember/render-modifiers #714

Merged
merged 7 commits into from
Nov 13, 2023

Conversation

gilest
Copy link
Contributor

@gilest gilest commented Oct 22, 2023

Replaces the no-longer-recommended render lifecycle modifiers with local function modifiers.

@ember/render-modifiers were meant as a transitional tool from the pre-Octane era, and not long-term usage.

Modifier code is directly ported from the setup/teardown hooks with no changes except what I've pointed out in code comments.

Recommend reviewing commit-by-commit.

}
}
// @ts-ignore
}, { eager: false });
Copy link
Contributor Author

@gilest gilest Oct 23, 2023

Choose a reason for hiding this comment

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

All modifier() invocations must include { eager: false } since this package supports ember-modifier v3.2.x and v4.

For apps using v3.2.x { eager: false } will ensure the default autotracking behaviour (eager).

For apps using v4 this argument will be ignored.

Comment on lines +164 to +166
// Escape autotracking frame and avoid backtracking re-render
Promise.resolve().then(() => {
this.args.dropdown.actions.reposition()
Copy link
Contributor Author

@gilest gilest Oct 23, 2023

Choose a reason for hiding this comment

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

This is the only code change I made. Since the did-insert modifier did not participate in auto tracking this was not an issue before.

I'm reasonably confident that this change is compatible since the tests pass. If this modifier doesn't run a number of tests will fail.

Someone more familiar with this library may have a better suggestion to replacing this operation with a derived data pattern (since this is a side-effect type modifier).

@gilest
Copy link
Contributor Author

gilest commented Nov 10, 2023

@mkszepp would you mind reviewing this?

@mkszepp mkszepp merged commit c7516fa into cibernox:master Nov 13, 2023
16 checks passed
@mkszepp
Copy link
Collaborator

mkszepp commented Nov 13, 2023

@gilest sry for my delay. it looks good. Thanks

@gilest gilest deleted the chore/replace-render-modifiers branch November 13, 2023 08:01
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