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.

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

HMR doesn't call componentDidUnload lifecycle hook #1316

Closed
simonhaenisch opened this issue Jan 15, 2019 · 6 comments
Closed

HMR doesn't call componentDidUnload lifecycle hook #1316

simonhaenisch opened this issue Jan 15, 2019 · 6 comments

Comments

@simonhaenisch
Copy link
Contributor

Stencil version:

 @stencil/core@0.16.2

I'm submitting a:

[x] bug report
[ ] feature request
[ ] support request

Current behavior:

I use this.interval = setInterval(...) in the componentWillLoad hook, and clearInterval(this.interval) in the componentWillUnload hook. When that component gets replaced by HMR, I have two intervals running, because apparently the componentWillLoad hook gets executed again, but the componentDidUnload hook doesn't.

Expected behavior:

When a component gets replaced by HMR, it should probably call the componentWillUnload hook of the old component instance that gets replaced? Not sure whether that introduces issues with keeping the component state, but imo it shouldn't.

Related code:

I wrote this example component to showcase it:

import { Component, State } from '@stencil/core';

const handleError = console.error;

const names = ['Foo', 'Bar', 'Foobar'];

const getRandomNameThatIsNot = async (excludeName: string) =>
  new Promise<string>(resolve => {
    const availableNames = names.filter(name => name !== excludeName);
    const randomIndex = Math.floor(Math.random() * availableNames.length);

    resolve(availableNames[randomIndex]);
  });

@Component({ tag: 'a-cmp' })
export class ACmp {
  @State() name = 'Barfoo';

  interval: any;

  componentWillLoad() {
    this.interval = setInterval(async () => this.load(), 2000);
  }

  componentDidUnload() {
    clearInterval(this.interval);
  }

  async load() {
    console.log('name:', this.name);

    const anotherName = await getRandomNameThatIsNot(this.name).catch(handleError);

    if (anotherName) {
      this.name = anotherName;
    }
  }

  render = () => <h1>Hello, {this.name}!</h1>;
}

Steps to reproduce:

  • Put the example in a file
  • Run Stencil in dev/watch mode
  • Load the component
  • Save the component file again to trigger HMR
  • The component will now log name: undefined along with the log of the second interval, because the first interval has not been cleared (the componentWillUnload hook wasn't executed)
  • Re-saving creates a third interval, and so on
@ionitron-bot ionitron-bot bot added the triage label Jan 15, 2019
@mikaelkaron
Copy link

I have a similar issue with https://github.com/mikaelkaron/stencil-apexcharts/blob/master/src/components/apex-chart/apex-chart.tsx and with some help I’ve resorted to marking this.chart with @State for now.

@Matthew-Smith
Copy link
Contributor

This is still an issue I'm seeing on 1.0.0-beta.8. I closed this duplicate report: #1571

@Sohail05
Copy link

Had the same issue and tested with a MutationObserver.
Works with dev server's HMR .

    @Element() el: HTMLSpectatorInteractionElement;

    componentDidLoad(){
       
        // Retrieve IntervalHandle
        const intervalHandle = setInterval( ()=>{console.log("Ping")} , 1000)
       
        // Create MO with callback
        const observer = new MutationObserver((m,o)=>{
            console.log("Mutation!",m,o);
            // Remove Interval
            clearInterval(IntervalHandle);
            observer.disconnect();
        });
       
        // Set subject/target to this component native element
        observer.observe(this.el, { attributes: true, childList: false, subtree: false });
    }

@zastrowm
Copy link

zastrowm commented Feb 7, 2020

Not a deep dive, but looking at the source, should disconnectedCallback be called in hmrStart?:

export const hmrStart = (elm: d.HostElement, cmpMeta: d.ComponentRuntimeMeta, hmrVersionId: string) => {
  // ¯\_(ツ)_/¯
  const hostRef = getHostRef(elm);

+  // call disconnectedCallback(elm); here?
+  disconnectedCallback(elm);

  // reset state flags to only have been connected
  hostRef.$flags$ = HOST_FLAGS.hasConnected;

  // TODO
  // detatch any event listeners that may have been added
  // because we're not passing an exact event name it'll
  // remove all of this element's event, which is good

  // create a callback for when this component finishes hmr
  elm['s-hmr-load'] = () => {
    // finished hmr for this element
    delete elm['s-hmr-load'];
    hmrFinish(elm, cmpMeta);
  };

  // re-initialize the component
  initializeComponent(elm, hostRef, cmpMeta, hmrVersionId);
};

@sethlivingston
Copy link

The documentation is unclear as to whether it should be disconnectedCallback or componentDidUnload. See https://stenciljs.com/docs/component-lifecycle

It sounds like it should be the former.

@splitinfinities
Copy link
Contributor

Hey there, thank you for the patience getting back to you. The new team is getting started and we're working through the backlog now.

We do not support Stencil versions less than v2, and you seem to be using an older version of Stencil. componentDidUnload has been removed since v2 in favor of disconnectedCallback, for its reliability. If you can upgrade to the latest version and let me know if this issue still exists, I would appreciate it.

Let me know if you need anything else!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants