feat(modifier-managers): use 3.22 capabilities #25

NullVoxPopuli · 2020-10-15T16:06:23Z

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

rwjblue

In general, this can be done without a breaking change. Bring in either ember-compatibility-helpers or @embroider/macros and use the appropriate manager API version based on the running Ember version.

Something like:

import { gte } from 'ember-compatibility-helpers';
import { setModifierManager, capabilities } from '@ember/modifier';

export default setModifierManager(
  () => ({
    capabilities: capabilities(gte('3.22.0') ? '3.22' : '3.13', {
      disableAutoTracking: true,
    }),

    createModifier() {},

    installModifier(_state, element, args) {
      let [fn, ...positional] = args.positional;

      fn(element, positional, args.named);
    },

    updateModifier() {},
    destroyModifier() {},
  }),
  class DidInsertModifier {}
);

.travis.yml

addon/modifiers/did-insert.js

addon/modifiers/did-update.js

addon/modifiers/will-destroy.js

package.json

tests/integration/modifiers/did-update-test.js

rwjblue · 2020-10-16T22:24:29Z

addon/modifiers/did-update.js

@@ -58,7 +59,7 @@ import { setModifierManager, capabilities } from '@ember/modifier';
 */
 export default setModifierManager(
  () => ({
-    capabilities: capabilities('3.13', { disableAutoTracking: true }),
+    capabilities: capabilities(gte('3.22.0') ? '3.22' : '3.13', { disableAutoTracking: true }),


Need to chat with @pzuraq about this one (did-insert and will-destroy seem fine). Specifically RE: disableAutoTracking. If we move to using 3.22 version of modifier managers we would not get rerendered when our own args change (kinda the point of did-update IMHO).

I don't know what the intersection of using the args proxy and using disableAutoTracking will do...

My guess is that this is why the tests are failing in 3.22:

not ok 5 Chrome 62.0 - [61 ms] - Integration | Modifier | did-update: it basically works --- actual: > null stack: > at Object.<anonymous> (http://localhost:7357/assets/tests.js:167:21) at processModule (http://localhost:7357/assets/test-support.js:3963:17) at module$1 (http://localhost:7357/assets/test-support.js:3988:4) at Module.callback (http://localhost:7357/assets/tests.js:165:21) at Module.exports (http://localhost:7357/assets/vendor.js:111:32) at requireModule (http://localhost:7357/assets/vendor.js:32:18) at TestLoader.<anonymous> (http://localhost:7357/assets/test-support.js:14870:11) at TestLoader.require (http://localhost:7357/assets/test-support.js:14860:27) message: > Expected 4 assertions, but 0 were run negative: > false browser log: |

I think I figured it out?

rwjblue · 2020-10-18T18:14:26Z

addon/modifiers/did-update.js

-    installModifier(state, element) {
+
+    installModifier(instance, element, args) {
      // save element into state bucket
-      state.element = element;
+      instance.element = element;


Please revert this. The name of the first variable should not be instance (bucket is what we commonly use, but state is still better than bucket).

does that mean that this should change to state? https://github.com/ember-modifier/ember-modifier/blob/master/addon/-private/class/modifier-manager.ts#L32

NullVoxPopuli · 2020-10-18T19:11:58Z

travis build for ember 3.12 needs to be re-run I think? it crashed before the actual relevant-parts-to-this-PR began

NullVoxPopuli · 2020-10-18T20:10:23Z

green!

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

…te-test changes

rwjblue · 2020-10-19T03:04:01Z

.travis.yml

@@ -3,10 +3,9 @@ language: node_js
 node_js:
  # we recommend testing addons with the same minimum supported node version as Ember CLI
  # so that your addon works for all apps
-  - "6"


Needs to land in the other PR first

rwjblue · 2020-10-19T03:05:38Z

addon/modifiers/did-update.js

+        // consume all args
+        for (let i = 0; i < args.positional.length; i++) {
+          // "noop" / consume the arg
+          args.positional[i];
+        }
+
+        for (let key of Object.keys(args.named)) {
+          // "noop" / consume the arg
+          args.named[key];
+        }


Ya, we definitely shouldn't do this. We should either stick with 3.13 capabilities (which does this for us) or fix the tests to actual consume.

@pzuraq what did the RFC say about rerunning RE: the arguments changes? Does it mention auto tracking (and therefore infer that consumption is required)?

Ya, we definitely shouldn't do this.

I don't think a did-update hook is possible to exist then :-/ (with capabilities 3.22)
it order for the update to run, tracked things must be consumed during install.

We should either stick with 3.13 capabilities

this might be a good option. More discouraging of ember-render-modifiers to last into long-lived code?

this very much a wip, but I'm gonna be doing these sorts of tests for ember-modifier, too: NullVoxPopuli#1

this very much a wip

Moved to draft state

I don't think a did-update hook is possible to exist then :-/ (with capabilities 3.22)

Works fine, you just have to consume what you care about.

Where do you do that consumption? Did-update will not consume without running, and it won't run without an update. That leaves the only option to consume during install.

Moved to draft state

I meant the linked PR :/

NullVoxPopuli · 2020-10-19T22:07:20Z

After talking with @pzuraq about this, I'm gonna close it :D

It doesn't make sense to use 3.22 capabilities, since this addon is an anti-pattern in modern code anyway. :D

This fixes a deprecation and removes the anti-pattern. See emberjs/ember-render-modifiers#25.

SergeAstapov · 2021-05-05T02:12:34Z

this addon is an anti-pattern in modern code

@NullVoxPopuli do you mind to share some info about that statement? Seems waky to hear that about what guides do teach us in https://guides.emberjs.com/release/components/template-lifecycle-dom-and-modifiers/#toc_calling-methods-on-first-render

NullVoxPopuli force-pushed the use-3-22-capabilities branch from 3ac6f20 to 3c0ccc9 Compare October 16, 2020 01:00

rwjblue requested changes Oct 16, 2020

View reviewed changes

NullVoxPopuli force-pushed the use-3-22-capabilities branch from 3c0ccc9 to 267ef12 Compare October 16, 2020 22:08

rwjblue reviewed Oct 16, 2020

View reviewed changes

NullVoxPopuli force-pushed the use-3-22-capabilities branch from 267ef12 to 8c12abf Compare October 18, 2020 14:23

This was referenced Oct 18, 2020

re-apply blueprint #27

Closed

[Bug] (capabilities 3.22) Custom modifier-manager#update not called when tracked property is updated emberjs/ember.js#19215

Closed

NullVoxPopuli force-pushed the use-3-22-capabilities branch from 8c12abf to b757d8a Compare October 18, 2020 18:10

rwjblue reviewed Oct 18, 2020

View reviewed changes

NullVoxPopuli force-pushed the use-3-22-capabilities branch from 16b1319 to d2d310f Compare October 18, 2020 18:29

NullVoxPopuli requested a review from rwjblue October 18, 2020 18:50

NullVoxPopuli force-pushed the use-3-22-capabilities branch from d7972e7 to e530408 Compare October 18, 2020 19:53

NullVoxPopuli added 3 commits October 18, 2020 22:06

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

update eslint to support decorators and run --fix on the new did-upda…

9b9cc9a

…te-test changes

change travis to node 10 so that eslint doesn't break on arg-less catch

6c06e0b

NullVoxPopuli force-pushed the use-3-22-capabilities branch from 9356885 to 6c06e0b Compare October 19, 2020 02:07

rwjblue requested changes Oct 19, 2020

View reviewed changes

rwjblue marked this pull request as draft October 19, 2020 14:16

NullVoxPopuli marked this pull request as ready for review October 19, 2020 15:01

NullVoxPopuli closed this Oct 19, 2020

rwjblue mentioned this pull request Mar 5, 2021

Update manager capabilities to 3.22 to fix deprecation in Ember 3.26 #33

Closed

nbibler added a commit to railsdiff/railsdiff that referenced this pull request Apr 9, 2021

Remove @ember/render-modifiers

2654723

This fixes a deprecation and removes the anti-pattern. See emberjs/ember-render-modifiers#25.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(modifier-managers): use 3.22 capabilities #25

feat(modifier-managers): use 3.22 capabilities #25

NullVoxPopuli commented Oct 15, 2020

rwjblue left a comment

rwjblue Oct 16, 2020

rwjblue Oct 16, 2020

NullVoxPopuli Oct 18, 2020

rwjblue Oct 18, 2020

NullVoxPopuli Oct 18, 2020

NullVoxPopuli Oct 18, 2020 •

edited

Loading

NullVoxPopuli commented Oct 18, 2020

NullVoxPopuli commented Oct 18, 2020

rwjblue Oct 19, 2020

rwjblue Oct 19, 2020

NullVoxPopuli Oct 19, 2020 •

edited

Loading

NullVoxPopuli Oct 19, 2020

rwjblue Oct 19, 2020 •

edited

Loading

NullVoxPopuli Oct 19, 2020

NullVoxPopuli commented Oct 19, 2020

SergeAstapov commented May 5, 2021

feat(modifier-managers): use 3.22 capabilities #25

feat(modifier-managers): use 3.22 capabilities #25

Conversation

NullVoxPopuli commented Oct 15, 2020

rwjblue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli Oct 18, 2020 • edited Loading

Choose a reason for hiding this comment

NullVoxPopuli commented Oct 18, 2020

NullVoxPopuli commented Oct 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli Oct 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwjblue Oct 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Oct 19, 2020

SergeAstapov commented May 5, 2021

NullVoxPopuli Oct 18, 2020 •

edited

Loading

NullVoxPopuli Oct 19, 2020 •

edited

Loading

rwjblue Oct 19, 2020 •

edited

Loading