can-component `beforeremove`-event does not fire with can-zone@0.6.13

cleong-tc commented 6 years ago

can-zone@0.6.13 added Node.prototype.removeEventListener to register.js which caused can-component/can-control beforeremove-event to not fire. https://v3.canjs.com/doc/can-component/beforeremove.html

it seems the beforeremove-event-handlers are being removed by Node.prototype.removeEventListener() (in can-simple-dom/lib/simple-dom/event.js) before Node.prototype.dispatchEvent() can call the beforeremove-event-handlers. Node.prototype.removeEventListener() is removing the beforeremove-event-handlers because if (handlersByType[index] === handler) { evaluates to true for can-zone@0.6.13 (vs false for can-zone@0.6.12).

in my donejs@1 sample app, with can-zone@0.6.12 the log-message for the beforeremove-event is displayed in the terminal-console, but with can-zone@0.6.13 the log-message is missing. we use the beforeremove-event to cleanup timers and element-event-bindings.

src/index.stache...

<html>
  <head>
  </head>
  <body>
        <can-import from="my-app/app" export-as="viewModel" />
        <can-import from="my-app/mobile/index/" />
        <mobile-index />

    {{#switch env.NODE_ENV}}
      {{#case "production"}}
        <script src="{{joinBase 'steal.production.js'}}"></script>
      {{/case}}
      {{#default}}
        <script src="/node_modules/steal/steal.js"></script>
      {{/default}}
    {{/switch}}
  </body>
</html>

src/mobile/index/index.js...

import Component from 'can-component';
import DefineMap from 'can-define/map/';
import view from './index.stache';

export const ViewModel = DefineMap.extend({
});

export default Component.extend({
  tag: 'mobile-index',
  ViewModel,
    view,
    events: {
        'inserted'() {
            console.log('mobile-index.events.inserted');
        },
        'removed'() {
            console.log('mobile-index.events.removed');
        },
        '{element} beforeremove'() {
            console.log('mobile-index.events.element.beforeremove');
        },
    }
});

package.json...

  "dependencies": {
    "commander": "^2.8.1",
    "debug": "^2.6.3",
    "express": "^4.13.4",
    "http-proxy": "^1.13.2",
    "can-component": "^3.3.4",
    "can-define": "^1.4.4",
    "can-route": "^3.3.4",
    "can-route-pushstate": "^3.1.0",
    "can-set": "^1.3.2",
    "can-stache": "^3.6.2",
    "can-view-import": "=3.2.1",
    "can-view-autorender": "^3.1.1",
    "can-util": "^3.9.10",
    "can-zone": "=0.6.13",
    "done-autorender": "^1.2.0",
    "done-component": "^1.0.0",
    "done-css": "^3.0.1",
    "done-ssr": "=1.1.5",
    "done-ssr-middleware": "=1.1.0",
    "steal": "^1.12.3",
    "steal-less": "^1.3.1",
    "steal-stache": "^3.1.2",
    "steal-tools": "^1.11.9"
  },

note: there is a version-lock for can-view-import@3.2.1 to resolve a memory-leak-issue because of can-view-import-promise.

matthewp commented 6 years ago

Thanks, I'll see if I can recreate.

matthewp commented 6 years ago

I created a glitch: https://glitch.com/edit/#!/sophisticated-turkey?path=server.js:13:0

What I see there seems to work:

Using your deps list. What's different?

cleong-tc commented 6 years ago

@matthewp : in package.json change "can-zone": "=0.6.13" to "can-zone": "=0.6.12" and you should see the additional terminal-console-message mobile-index.events.element.beforeremove.

sorry, i am not familiar with glitch.com. i need to figure out how to see the console-output.

matthewp commented 6 years ago

Ah, I see. Interesting... ok, this gives me something to work with so I'll check it out.

cleong-tc commented 6 years ago

@matthewp : beforeremove-event seems to be firing in done-ssr@2, so i am fine with ignoring this issue.

matthewp commented 6 years ago

Hm, so probably not a bug in can-zone but rather more likely in done-ssr. Trying to think if we had tests with beforeremove...

canjs / can-zone

can-component `beforeremove`-event does not fire with can-zone@0.6.13 #178