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

Prepare for ember-modifier v4 #617

Merged
merged 1 commit into from
Apr 7, 2022
Merged

Conversation

SergeAstapov
Copy link
Contributor

@SergeAstapov SergeAstapov commented Apr 6, 2022

Follows ember-modifier v4 migration guide to remove deprecations added in v3.2.0 and makes addon future ready.

Follows [`ember-modifier` v4 migration guide](https://github.com/ember-modifier/ember-modifier/blob/v3.2.5/MIGRATIONS.md#function-based-modifiers)
to remove deprecations added in v3.2.0 and makes addon future ready.
this.element.addEventListener('focusout', this.onFocusOut, true);
let modifier;

if (macroCondition(dependencySatisfies('ember-modifier', '>=3.2.0 || 4.x'))) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This increases code complexity.

However, my thought process is following:

  • with such condition, only one version of the modifier class gets into bundle.
  • we'll drop older version of ember-modifier support at some point, so one branch will just be deleted
  • and most important IMO, it would be messy to use either modify or didReceiveArguments/didInstall/willRemove in the same class.
    Hence having two separate classes is simpler to handle.

Copy link
Collaborator

@lukemelia lukemelia left a comment

Choose a reason for hiding this comment

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

This approach seems sound to me 👍

@SergeAstapov SergeAstapov merged commit 4a3615b into master Apr 7, 2022
@SergeAstapov SergeAstapov deleted the ember-modifier-v4-compat branch April 7, 2022 03:05
@SergeAstapov
Copy link
Contributor Author

@lukemelia as this was merged, could you please publish v8.1.0? this is enhancement hence I assume it's "minor" rather than "patch"

@lukemelia
Copy link
Collaborator

Done! Thanks, @SergeAstapov

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.

3 participants