Skip to content

Commit

Permalink
feat(componentDidUnload): use disconnectedCallback instead of compone…
Browse files Browse the repository at this point in the history
…ntDidUnload

When Stencil is used within other frameworks, elements may be reused, making it impossible for componentDidUnload to be accurate 100% of the time if it is disconnected more than once. Instead, disconnectedCallback() is the preferred way to always know if a component was disconnected from the DOM.

Note that the runtime still works for any collections that have been built with componentDidUnload(). However, updates to Stencil 2 will require it's changed to disconnectedCallback().
  • Loading branch information
adamdbradley committed Aug 5, 2020
1 parent 1a94d1e commit 4e45862
Show file tree
Hide file tree
Showing 13 changed files with 59 additions and 60 deletions.
7 changes: 2 additions & 5 deletions STYLE_GUIDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -209,13 +209,10 @@ export class Something {
* Ordered by their natural call order, for example
* WillLoad should go before DidLoad.
*/
connectedCallback() {}
componentWillLoad() {}
componentDidLoad() {}
componentWillEnter() {}
componentDidEnter() {}
componentWillLeave() {}
componentDidLeave() {}
componentDidUnload() {}
disconnectedCallbac() {}

/**
* 8. Listeners
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { elementDecoratorsToStatic } from './element-decorator';
import { eventDecoratorsToStatic } from './event-decorator';
import { listenDecoratorsToStatic } from './listen-decorator';
import { isDecoratorNamed } from './decorator-utils';
import { methodDecoratorsToStatic } from './method-decorator';
import { methodDecoratorsToStatic, validateMethods } from './method-decorator';
import { propDecoratorsToStatic } from './prop-decorator';
import { stateDecoratorsToStatic } from './state-decorator';
import { watchDecoratorsToStatic } from './watch-decorator';
Expand Down Expand Up @@ -36,8 +36,9 @@ export const visitClassDeclaration = (config: d.Config, diagnostics: d.Diagnosti
return classNode;
}

const decoratedMembers = classNode.members.filter(member => Array.isArray(member.decorators) && member.decorators.length > 0);
const newMembers = removeStencilDecorators(Array.from(classNode.members));
const classMembers = classNode.members;
const decoratedMembers = classMembers.filter(member => Array.isArray(member.decorators) && member.decorators.length > 0);
const newMembers = removeStencilDecorators(Array.from(classMembers));

// parser component decorator (Component)
componentDecoratorToStatic(config, typeChecker, diagnostics, classNode, newMembers, componentDecorator);
Expand All @@ -54,6 +55,8 @@ export const visitClassDeclaration = (config: d.Config, diagnostics: d.Diagnosti
listenDecoratorsToStatic(diagnostics, decoratedMembers, newMembers);
}

validateMethods(diagnostics, classMembers);

return ts.updateClassDeclaration(
classNode,
removeDecorators(classNode, CLASS_DECORATORS_TO_REMOVE),
Expand Down
11 changes: 11 additions & 0 deletions src/compiler/transformers/decorators-to-static/method-decorator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,3 +89,14 @@ const parseMethodDecorator = (config: d.Config, diagnostics: d.Diagnostic[], tsS
const isTypePromise = (typeStr: string) => {
return /^Promise<.+>$/.test(typeStr);
};

export const validateMethods = (diagnostics: d.Diagnostic[], members: ts.NodeArray<ts.ClassElement>) => {
members.filter(ts.isMethodDeclaration).map(method => {
if (method.name.getText() === 'componentDidUnload') {
const err = buildError(diagnostics);
err.header = `Replace "componentDidUnload()" with "disconnectedCallback()"`;
err.messageText = `The "componentDidUnload()" method was removed in Stencil 2. Please use the "disconnectedCallbac()" method instead.`;
augmentDiagnosticWithNode(err, method.name);
}
});
};
1 change: 0 additions & 1 deletion src/compiler/transformers/static-to-meta/component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ export const parseStaticComponentMeta = (
const encapsulation = parseStaticEncapsulation(staticMembers);

const cmp: d.ComponentCompilerMeta = {
isLegacy: false,
tagName: tagName,
excludeFromCollection: moduleFile.excludeFromCollection,
isCollectionDependency,
Expand Down
23 changes: 6 additions & 17 deletions src/compiler/transpile/run-program.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
import * as d from '../../declarations';
import { basename, join, relative } from 'path';
import { buildError, loadTypeScriptDiagnostics, normalizePath } from '@utils';
import type * as d from '../../declarations';
import { basename, join } from 'path';
import { convertDecoratorsToStatic } from '../transformers/decorators-to-static/convert-decorators';
import { generateAppTypes } from '../types/generate-app-types';
import { getComponentsFromModules, isOutputTargetDistTypes } from '../output-targets/output-utils';
import { loadTypeScriptDiagnostics, normalizePath } from '@utils';
import { resolveComponentDependencies } from '../entries/resolve-component-dependencies';
import type ts from 'typescript';
import { updateComponentBuildConditionals } from '../app-core/app-data';
import { updateModule } from '../transformers/static-to-meta/parse-static';
import ts from 'typescript';
import { updateStencilTypesImports } from '../types/stencil-types';
import { validateTranspiledComponents } from './validate-components';

export const runTsProgram = async (config: d.Config, compilerCtx: d.CompilerCtx, buildCtx: d.BuildCtx, tsBuilder: ts.BuilderProgram) => {
const tsSyntactic = loadTypeScriptDiagnostics(tsBuilder.getSyntacticDiagnostics());
Expand Down Expand Up @@ -54,7 +55,7 @@ export const runTsProgram = async (config: d.Config, compilerCtx: d.CompilerCtx,
updateComponentBuildConditionals(compilerCtx.moduleMap, buildCtx.components);
resolveComponentDependencies(buildCtx.components);

validateUniqueTagNames(config, buildCtx);
validateTranspiledComponents(config, buildCtx);

if (buildCtx.hasError) {
return false;
Expand Down Expand Up @@ -82,18 +83,6 @@ export const runTsProgram = async (config: d.Config, compilerCtx: d.CompilerCtx,
return false;
};

const validateUniqueTagNames = (config: d.Config, buildCtx: d.BuildCtx) => {
buildCtx.components.forEach(cmp => {
const tagName = cmp.tagName;
const cmpsWithTagName = buildCtx.components.filter(c => c.tagName === tagName);
if (cmpsWithTagName.length > 1) {
const err = buildError(buildCtx.diagnostics);
err.header = `Component Tag Name "${tagName}" Must Be Unique`;
err.messageText = `Please update the components so "${tagName}" is only used once: ${cmpsWithTagName.map(c => relative(config.rootDir, c.sourceFilePath)).join(' ')}`;
}
});
};

const getRelativeDts = (config: d.Config, srcPath: string, emitDtsPath: string) => {
const parts: string[] = [];
srcPath = normalizePath(srcPath);
Expand Down
19 changes: 19 additions & 0 deletions src/compiler/transpile/validate-components.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import type * as d from '../../declarations';
import { buildError } from '@utils';
import { relative } from 'path';

export const validateTranspiledComponents = (config: d.Config, buildCtx: d.BuildCtx) => {
for (const cmp of buildCtx.components) {
validateUniqueTagNames(config, buildCtx, cmp);
}
};

const validateUniqueTagNames = (config: d.Config, buildCtx: d.BuildCtx, cmp: d.ComponentCompilerMeta) => {
const tagName = cmp.tagName;
const cmpsWithTagName = buildCtx.components.filter(c => c.tagName === tagName);
if (cmpsWithTagName.length > 1) {
const err = buildError(buildCtx.diagnostics);
err.header = `Component Tag Name "${tagName}" Must Be Unique`;
err.messageText = `Please update the components so "${tagName}" is only used once: ${cmpsWithTagName.map(c => relative(config.rootDir, c.sourceFilePath)).join(' ')}`;
}
};
8 changes: 0 additions & 8 deletions src/compiler/types/generate-app-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ const generateComponentTypesFile = (config: d.Config, buildCtx: d.BuildCtx, inte
let typeImportData: d.TypesImportData = {};
const c: string[] = [];
const allTypes = new Map<string, number>();
const needsJSXElementHack = buildCtx.components.some(cmp => cmp.isLegacy);
const components = buildCtx.components.filter(m => !m.isCollectionDependency);

const modules: d.TypesModule[] = components.map(cmp => {
Expand Down Expand Up @@ -83,13 +82,6 @@ const generateComponentTypesFile = (config: d.Config, buildCtx: d.BuildCtx, inte

c.push(`declare global {`);

if (needsJSXElementHack) {
c.push(` // Adding a global JSX for backcompatibility with legacy dependencies`);
c.push(` export namespace JSX {`);
c.push(` export interface Element {}`);
c.push(` }`);
}

c.push(...modules.map(m => m.element));

c.push(` interface HTMLElementTagNameMap {`);
Expand Down
1 change: 0 additions & 1 deletion src/declarations/stencil-core/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ export {
Component,
ComponentOptions,
ComponentDidLoad,
ComponentDidUnload,
ComponentDidUpdate,
ComponentInterface,
ComponentWillLoad,
Expand Down
1 change: 0 additions & 1 deletion src/declarations/stencil-private.ts
Original file line number Diff line number Diff line change
Expand Up @@ -693,7 +693,6 @@ export interface ComponentCompilerMeta extends ComponentCompilerFeatures {
shadowDelegatesFocus: boolean;
excludeFromCollection: boolean;
isCollectionDependency: boolean;
isLegacy: boolean;
docs: CompilerJsDoc;
jsFilePath: string;
listeners: ComponentCompilerListener[];
Expand Down
8 changes: 0 additions & 8 deletions src/declarations/stencil-public-runtime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -363,14 +363,6 @@ export interface ComponentDidUpdate {
componentDidUpdate(): void;
}

export interface ComponentDidUnload {
/**
* The component did unload and the element
* will be destroyed.
*/
componentDidUnload(): void;
}

export interface ComponentInterface {
connectedCallback?(): void;
disconnectedCallback?(): void;
Expand Down
15 changes: 7 additions & 8 deletions test/karma/test-app/dom-reattach/cmp-root.tsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
import { Component, h, Host, Prop } from '@stencil/core';

@Component({
tag: 'dom-reattach'
tag: 'dom-reattach',
})
export class DomReattach {

@Prop({mutable: true}) willLoad = 0;
@Prop({mutable: true}) didLoad = 0;
@Prop({mutable: true}) didUnload = 0;
@Prop({ mutable: true }) willLoad = 0;
@Prop({ mutable: true }) didLoad = 0;
@Prop({ mutable: true }) didUnload = 0;

componentWillLoad() {
this.willLoad++;
Expand All @@ -17,7 +16,7 @@ export class DomReattach {
this.didLoad++;
}

componentDidUnload() {
disconnectedCallback() {
this.didUnload++;
}

Expand All @@ -26,8 +25,8 @@ export class DomReattach {
<Host>
<p>componentWillLoad: {this.willLoad}</p>
<p>componentDidLoad: {this.didLoad}</p>
<p>componentDidUnload: {this.didUnload}</p>
<p>disconnectedCallback: {this.didUnload}</p>
</Host>
)
);
}
}
5 changes: 2 additions & 3 deletions test/karma/test-app/lifecycle-unload/cmp-a.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,17 @@ import { Component, Element, h } from '@stencil/core';

@Component({
tag: 'lifecycle-unload-a',
shadow: true
shadow: true,
})
export class LifecycleUnloadA {

@Element() el!: HTMLElement;
results?: HTMLDivElement | null;

componentDidLoad() {
this.results = this.el.ownerDocument!.body.querySelector('#lifecycle-unload-results') as HTMLDivElement;
}

componentDidUnload() {
disconnectedCallback() {
const elm = document.createElement('div');
elm.textContent = 'cmp-a unload';
this.results!.appendChild(elm);
Expand Down
11 changes: 6 additions & 5 deletions test/karma/test-app/lifecycle-unload/cmp-b.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,17 @@ import { Component, Element, h } from '@stencil/core';

@Component({
tag: 'lifecycle-unload-b',
shadow: true
shadow: true,
})
export class LifecycleUnloadB {

@Element() el!: HTMLElement;
results?: HTMLDivElement;

componentDidLoad() {
this.results = this.el.ownerDocument!.body.querySelector('#lifecycle-unload-results') as HTMLDivElement;
}

componentDidUnload() {
disconnectedCallback() {
const elm = document.createElement('div');
elm.textContent = 'cmp-b unload';
this.results!.appendChild(elm);
Expand All @@ -22,8 +21,10 @@ export class LifecycleUnloadB {
render() {
return [
<article>cmp-b - top</article>,
<section><slot/></section>,
<nav>cmp-b - bottom</nav>
<section>
<slot />
</section>,
<nav>cmp-b - bottom</nav>,
];
}
}

0 comments on commit 4e45862

Please sign in to comment.