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

destroyModifier is called in SSR mode #17949

Closed
josemarluedke opened this issue Apr 18, 2019 · 8 comments · Fixed by #18071
Closed

destroyModifier is called in SSR mode #17949

josemarluedke opened this issue Apr 18, 2019 · 8 comments · Fixed by #18071
Assignees
@rwjblue
Labels
Bug Octane
Milestone
3.10

Comments

@josemarluedke
Copy link
Contributor

josemarluedke commented Apr 18, 2019 •
edited
Loading

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
Copy link
Member

rwjblue commented Apr 19, 2019

Thank you for reporting (and the reproduction)!

@rwjblue rwjblue mentioned this issue Apr 19, 2019
Closed
7 tasks
@rwjblue rwjblue self-assigned this Apr 23, 2019
@rwjblue rwjblue added this to the 3.10 milestone Apr 30, 2019
@josemarluedke
Copy link
Contributor Author

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
Copy link

I've just run up against this issue.

Whereas installModifier and updateModifier are invoked if environment.isInteractive:

ember.js/packages/@ember/-internals/glimmer/lib/environment.ts

Lines 72 to 82 in 85de05f

scheduleInstallModifier(modifier: any, manager: any): void {
if (this.isInteractive) {
super.scheduleInstallModifier(modifier, manager);
}
}
scheduleUpdateModifier(modifier: any, manager: any): void {
if (this.isInteractive) {
super.scheduleUpdateModifier(modifier, manager);
}
}

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

ember.js/packages/@ember/-internals/glimmer/lib/modifiers/custom.ts

Lines 43 to 46 in 85de05f

destroy() {
const { delegate, modifier, args } = this;
delegate.destroyModifier(modifier, args.value());
}

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

@rwjblue rwjblue mentioned this issue May 28, 2019
Merged
@CvX
Copy link
Contributor

CvX commented May 29, 2019

@willviles thanks for the insight! Here's my stab at the problem: 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
Copy link
Member

rwjblue commented May 30, 2019

@CvX - Mind sending that in as a PR?

@rwjblue rwjblue mentioned this issue Jun 3, 2019
Merged
@rwjblue
Copy link
Member

rwjblue commented Jun 3, 2019

OK, #18071 should fix.

@rwjblue
Copy link
Member

rwjblue commented Jun 3, 2019

Thank you @CvX for pushing up those tests!

@CvX
Copy link
Contributor

CvX commented Jun 3, 2019

@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. 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rwjblue rwjblue

Labels
Bug Octane
Projects
None yet
Milestone
3.10
Development

Successfully merging a pull request may close this issue.

[BUGFIX lts + beta] Ensure modifiers do not run in FastBoot modes. rwjblue/ember.js
4 participants
@rwjblue @CvX @josemarluedke @willviles