Ember.js: destroyModifier is called in SSR mode

[josemarluedke]

Using modifiers with FastBoot (SSR) should result in them not been executed at all in FastBoot, leaving it to be only executed in client environment.

Currently: installModifier is not called in SSR, however, destroyModifier is.
Expected: destroyModifier to not be called in SSR.

Reproduction is here: https://github.com/josemarluedke/--ember-modiers-fastboot

[rwjblue]

Thank you for reporting (and the reproduction)!

[josemarluedke]

This issue is also present when using the on modifier from the feature flag under canary.

TypeError: element.removeEventListener is not a function
    at removeEventListener (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/@ember/-internals/glimmer.js:8859:1)
    at OnModifierState.destroy (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/@ember/-internals/glimmer.js:8832:1)
    at SimpleBlockTracker.destroy (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/@glimmer/runtime.js:3606:1)
    at SimpleBlockTracker.destroy (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/@glimmer/runtime.js:3606:1)
    at UpdatableBlockTracker.destroy (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/@glimmer/runtime.js:3606:1)
    at SimpleBlockTracker.destroy (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/@glimmer/runtime.js:3606:1)
    at UpdatableBlockTracker.destroy (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/@glimmer/runtime.js:3606:1)
    at SimpleBlockTracker.destroy (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/@glimmer/runtime.js:3606:1)
    at UpdatableBlockTracker.destroy (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/@glimmer/runtime.js:3606:1)
    at SimpleBlockTracker.destroy (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/@glimmer/runtime.js:3606:1)
    at RenderResult.destroy (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/@glimmer/runtime.js:4350:1)
    at RootState.destroy (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/@ember/-internals/glimmer.js:5912:1)
    at InertRenderer._clearAllRoots (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/@ember/-internals/glimmer.js:6226:1)
    at InertRenderer.destroy (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/@ember/-internals/glimmer.js:6096:1)
    at destroyDestroyables (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/@ember/-internals/container.js:439:1)
    at Container.destroy (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/@ember/-internals/container.js:142:1)
    at /var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/@ember/-internals/runtime/lib/mixins/container_proxy.js:86:1
    at Backburner._join (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/backburner.js:992:1)
    at Backburner.join (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/backburner.js:757:1)
    at join (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/@ember/runloop/index.js:172:1)
    at Class.destroy (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/@ember/-internals/runtime/lib/mixins/container_proxy.js:85:1)
    at Class.superWrapper [as destroy] (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/@ember/-internals/utils.js:366:1)
    at Result._destroyAppInstance (/Users/josemarluedke/code/neighborly/demand-stack/demand-web/node_modules/fastboot/src/result.js:144:21)
    at visitRoute.then.catch.then.finally (/Users/josemarluedke/code/neighborly/demand-stack/demand-web/node_modules/fastboot/src/ember-app.js:331:20)
    at promise.then.value (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/rsvp.js:1110:1)
    at tryCatcher (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/rsvp.js:335:1)
    at invokeCallback (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/rsvp.js:506:1)
    at publish (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/rsvp.js:492:1)
    at _runloop.backburner.schedule (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/ember-testing/lib/ext/rsvp.js:16:1)
    at invokeWithOnError (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/backburner.js:344:1)
    at Queue.flush (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/backburner.js:226:1)
    at DeferredActionQueues.flush (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/backburner.js:423:1)
    at Backburner._end (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/backburner.js:957:1)
    at Backburner._boundAutorunEnd (/var/folders/67/6jb5flcd6d30lkgvj0_92c_m0000gn/T/broccoli-875498bYpOlTJAjU2/out-475-broccoli_merge_trees/assets/backburner.js:626:1)
    at processTicksAndRejections (internal/process/next_tick.js:81:5)

[willviles]

I've just run up against this issue.

Whereas installModifier and updateModifier are invoked if environment.isInteractive:
https://github.com/emberjs/ember.js/blob/85de05ffc587f2efc0b6d4b011e7848f5c2679ed/packages/%40ember/-internals/glimmer/lib/environment.ts#L72-L82

destroyModifier is being invoked via the generic object destroy method on the ModifierManager without such a check:

https://github.com/emberjs/ember.js/blob/85de05ffc587f2efc0b6d4b011e7848f5c2679ed/packages/%40ember/-internals/glimmer/lib/modifiers/custom.ts#L43-L46

Not sure of the best solution of getting the isInteractive state from the environment in that particular context...

[CvX]

@willviles thanks for the insight! Here's my stab at the problem: https://github.com/emberjs/ember.js/compare/master...CvX:modifiers-destroy-non-interactive

The "bugfix" 😂 is atrocious and brittle, but it's a start:

a. it has tests, so at the very least that could lead to an actual fix
b. it works in my app 😉

[rwjblue]

@CvX - Mind sending that in as a PR?

[rwjblue]

OK, https://github.com/emberjs/ember.js/pull/18071 should fix.

[rwjblue]

Thank you @CvX for pushing up those tests!

[CvX]

@rwjblue ah, sorry for the slow response on my side! Thank you for the actual fix. 🙂 I'm checking out the code right now, and will try it out in my app in a moment. 🙂