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.

Sign up for GitHub

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

Jump to bottom

[BUGFIX beta] Use args proxy for modifier managers. #19163

Merged
merged 2 commits into from
Sep 28, 2020
Merged

[BUGFIX beta] Use args proxy for modifier managers. #19163

merged 2 commits into from
Sep 28, 2020

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Sep 28, 2020 •
edited
Loading

This PR does two main things:

  1. Generalizes the args proxy for use in components, helpers, and modifiers. This does change the semantics slightly of positional arguments for component managers, notably it would make it so using computed properties with them would require a bit more work from custom component managers. The new semantics are more inline with the future of Ember, but if there many custom managers that use positional args we may want to reconsider this change.
  2. Introduce the 3.22 manager API version to ensure that custom modifier managers only consume the arguments that are actually used by the manager. In order for specific modifiers to take advantage of these changes, they need to be updated to use modifierCapabilities('3.22') (instead of 3.13).

Fixes #19162

@rwjblue rwjblue added the Bug label Sep 28, 2020
@rwjblue rwjblue requested a review from pzuraq September 28, 2020 14:18
@rwjblue rwjblue mentioned this pull request Sep 28, 2020
Closed
@rwjblue rwjblue force-pushed the modifier-args-proxy branch 3 times, most recently from ec868d1 to 9a90d5d Compare September 28, 2020 14:33
@rwjblue rwjblue marked this pull request as ready for review September 28, 2020 14:37
@rwjblue
Copy link
Member Author

rwjblue commented Sep 28, 2020

Need to coordinate with @pzuraq to avoid stepping on his toes over in #19160 (which does similar things).

pzuraq and others added 2 commits September 28, 2020 14:13
@pzuraq @rwjblue
[BUGFIX beta] Generalize args proxy.
b49f91d 
 Generalizes the args proxy for use in both components and helpers.
 This does change the semantics slightly of `positional` arguments for
 component managers, notably it would make it so using computed
 properties with them would require a bit more work from custom
 component managers. The new semantics are more inline with the future
 of Ember, but if there many custom managers that use positional args we
 may want to reconsider this change.

Co-authored-by: Robert Jackson <me@rwjblue.com>
@rwjblue
[BUGFIX beta] Ensure custom modifiers use args proxy.
d7f56c4 
Prior to this change, all custom modifiers **always** consumed every
argument on install/update/destroy. This was because `reifyArgs`
specifically reads all of them, and the actual usage was not
consumption based.

After this change, any modifiers using `modifierCapabilities('3.22')`
will only consume the arguments that are actually used (named and
positional). All modifiers that use `modifierCapabilities('3.13')` are
unchanged (preserving backwards compatibility).
@rwjblue
Copy link
Member Author

rwjblue commented Sep 28, 2020

I've pulled in the changes from #19160 so we should be able to easily rebase that on top of these changes once this lands.

@rwjblue rwjblue changed the title [BUGFIX lts] Use args proxy for modifier managers. [BUGFIX beta] Use args proxy for modifier managers. Sep 28, 2020
pzuraq
pzuraq approved these changes Sep 28, 2020
View reviewed changes
Copy link
Contributor

@pzuraq pzuraq left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for doing this!

@rwjblue rwjblue merged commit aeef923 into master Sep 28, 2020
@rwjblue rwjblue deleted the modifier-args-proxy branch September 28, 2020 19:03
@pzuraq pzuraq mentioned this pull request Sep 29, 2020
Closed
@locks locks mentioned this pull request Oct 6, 2020
Merged
@NullVoxPopuli NullVoxPopuli mentioned this pull request Oct 15, 2020
Closed
NullVoxPopuli added a commit to NullVoxPopuli/ember-render-modifiers that referenced this pull request Oct 15, 2020
@NullVoxPopuli
feat(modifier-managers): use 3.22 capabilities
d8c6744 
BREAKING CHANGE: destruction behavior changed to use new args proxy from 3.22

Resolves the issue
  described by: emberjs/ember.js#19162
  fixed by: emberjs/ember.js#19163
@NullVoxPopuli NullVoxPopuli mentioned this pull request Oct 15, 2020
Closed
NullVoxPopuli added a commit to NullVoxPopuli/ember-modifier that referenced this pull request Oct 16, 2020
@NullVoxPopuli
feat(modifier-managers): use 3.22 capabilities
21e0abd 
BREAKING CHANGE: destruction behavior changed to use new args proxy from
3.22

Resolves the issue
  described by: emberjs/ember.js#19162
  fixed by: emberjs/ember.js#19163
NullVoxPopuli added a commit to NullVoxPopuli/ember-render-modifiers that referenced this pull request Oct 16, 2020
@NullVoxPopuli
feat(modifier-managers): use 3.22 capabilities
3c0ccc9 
BREAKING CHANGE: destruction behavior changed to use new args proxy from 3.22

Resolves the issue
  described by: emberjs/ember.js#19162
  fixed by: emberjs/ember.js#19163
NullVoxPopuli added a commit to NullVoxPopuli/ember-render-modifiers that referenced this pull request Oct 16, 2020
@NullVoxPopuli
feat(modifier-managers): use 3.22 capabilities
267ef12 
BREAKING CHANGE: destruction behavior changed to use new args proxy from 3.22

Resolves the issue
  described by: emberjs/ember.js#19162
  fixed by: emberjs/ember.js#19163

Ember apps < 3.22 are unaffected.

This also adds Ember Try support for LTS 3.8 through 3.20
NullVoxPopuli added a commit to NullVoxPopuli/ember-modifier that referenced this pull request Oct 16, 2020
@NullVoxPopuli
feat(modifier-managers): use 3.22 capabilities
1f119e7 
BREAKING CHANGE: destruction behavior changed to use new args proxy from
3.22

Resolves the issue
  described by: emberjs/ember.js#19162
  fixed by: emberjs/ember.js#19163
NullVoxPopuli added a commit to NullVoxPopuli/ember-modifier that referenced this pull request Oct 16, 2020
@NullVoxPopuli
feat(modifier-managers): use 3.22 capabilities
eccf42d 
BREAKING CHANGE: destruction behavior changed to use new args proxy from
3.22

Resolves the issue
  described by: emberjs/ember.js#19162
  fixed by: emberjs/ember.js#19163
NullVoxPopuli added a commit to NullVoxPopuli/ember-modifier that referenced this pull request Oct 16, 2020
@NullVoxPopuli
feat(modifier-managers): use 3.22 capabilities
cbb9344 
BREAKING CHANGE: destruction behavior changed to use new args proxy from
3.22

Resolves the issue
  described by: emberjs/ember.js#19162
  fixed by: emberjs/ember.js#19163
NullVoxPopuli added a commit to NullVoxPopuli/ember-modifier that referenced this pull request Oct 16, 2020
@NullVoxPopuli
feat(modifier-managers): use 3.22 capabilities
18ed5ba 
BREAKING CHANGE: destruction behavior changed to use new args proxy from
3.22

Resolves the issue
  described by: emberjs/ember.js#19162
  fixed by: emberjs/ember.js#19163
NullVoxPopuli added a commit to NullVoxPopuli/ember-render-modifiers that referenced this pull request Oct 18, 2020
@NullVoxPopuli
feat(modifier-managers): use 3.22 capabilities
5ef163d 
BREAKING CHANGE: destruction behavior changed to use new args proxy from 3.22

Resolves the issue
  described by: emberjs/ember.js#19162
  fixed by: emberjs/ember.js#19163

Ember apps < 3.22 are unaffected.

This also adds Ember Try support for LTS 3.8 through 3.20
@NullVoxPopuli NullVoxPopuli mentioned this pull request Oct 18, 2020
Closed
NullVoxPopuli added a commit to NullVoxPopuli/ember-render-modifiers that referenced this pull request Oct 18, 2020
@NullVoxPopuli
feat(modifier-managers): use 3.22 capabilities
b757d8a 
BREAKING CHANGE: destruction behavior changed to use new args proxy from 3.22

Resolves the issue
  described by: emberjs/ember.js#19162
  fixed by: emberjs/ember.js#19163
NullVoxPopuli added a commit to NullVoxPopuli/ember-render-modifiers that referenced this pull request Oct 18, 2020
@NullVoxPopuli
feat(modifier-managers): use 3.22 capabilities
1eb7c68 
BREAKING CHANGE: destruction behavior changed to use new args proxy from 3.22

Resolves the issue
  described by: emberjs/ember.js#19162
  fixed by: emberjs/ember.js#19163
NullVoxPopuli added a commit to NullVoxPopuli/ember-modifier that referenced this pull request Oct 18, 2020
@NullVoxPopuli
feat(modifier-managers): use 3.22 capabilities
6662268 
BREAKING CHANGE: destruction behavior changed to use new args proxy from
3.22

Resolves the issue
  described by: emberjs/ember.js#19162
  fixed by: emberjs/ember.js#19163
NullVoxPopuli added a commit to NullVoxPopuli/ember-modifier that referenced this pull request Oct 18, 2020
@NullVoxPopuli
feat(modifier-managers): use 3.22 capabilities
8d4827a 
BREAKING CHANGE: destruction behavior changed to use new args proxy from
3.22

Resolves the issue
  described by: emberjs/ember.js#19162
  fixed by: emberjs/ember.js#19163
NullVoxPopuli added a commit to NullVoxPopuli/ember-modifier that referenced this pull request Oct 18, 2020
@NullVoxPopuli
feat(modifier-managers): use 3.22 capabilities
bc0145a 
BREAKING CHANGE: destruction behavior changed to use new args proxy from
3.22

Resolves the issue
  described by: emberjs/ember.js#19162
  fixed by: emberjs/ember.js#19163
NullVoxPopuli added a commit to NullVoxPopuli/ember-modifier that referenced this pull request Oct 18, 2020
@NullVoxPopuli
feat(modifier-managers): use 3.22 capabilities
89b1d57 
BREAKING CHANGE: destruction behavior changed to use new args proxy from
3.22

Resolves the issue
  described by: emberjs/ember.js#19162
  fixed by: emberjs/ember.js#19163
NullVoxPopuli added a commit to NullVoxPopuli/ember-modifier that referenced this pull request Oct 18, 2020
@NullVoxPopuli
feat(modifier-managers): use 3.22 capabilities
ebd5e12 
BREAKING CHANGE: destruction behavior changed to use new args proxy from
3.22

Resolves the issue
  described by: emberjs/ember.js#19162
  fixed by: emberjs/ember.js#19163
NullVoxPopuli added a commit to NullVoxPopuli/ember-render-modifiers that referenced this pull request Oct 19, 2020
@NullVoxPopuli
feat(modifier-managers): use 3.22 capabilities
0f3ee07 
BREAKING CHANGE: destruction behavior changed to use new args proxy from 3.22

Resolves the issue
  described by: emberjs/ember.js#19162
  fixed by: emberjs/ember.js#19163
NullVoxPopuli added a commit to NullVoxPopuli/ember-modifier that referenced this pull request Oct 20, 2020
@NullVoxPopuli
feat(modifier-managers): use 3.22 capabilities
9aee8de 
BREAKING CHANGE: destruction behavior changed to use new args proxy from
3.22.
This is only a breaking change for Ember 3.22+. Ember < 3.22 is
  unaffected.

Resolves the issue
  described by: emberjs/ember.js#19162
  fixed by: emberjs/ember.js#19163
NullVoxPopuli added a commit to NullVoxPopuli/ember-modifier that referenced this pull request Oct 20, 2020
@NullVoxPopuli
feat(modifier-managers): use 3.22 capabilities
d65b6b6 
BREAKING CHANGE: destruction behavior changed to use new args proxy from
3.22

Resolves the issue
  described by: emberjs/ember.js#19162
  fixed by: emberjs/ember.js#19163
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pzuraq pzuraq pzuraq approved these changes

Assignees
No one assigned
Labels
Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Modifier Managers should not re-consume their args on destroy
2 participants
@rwjblue @pzuraq