Skip to content

Commit

Permalink
fix(compiler): handle ts 5.0 static members
Browse files Browse the repository at this point in the history
this commit fixes a bug where static class members that are intialized
on a stencil component result in 2 `export` statements of a component
class being generated. as a result of 2 `ExportDeclaration`s being
created, the compilation process will fail.

this fix involves detecting initialized static members of a class before
they go through the transpilation phase, because a class can be
transformed in such a way that we lose the ability to determine if
something like the following was originally authored:
```ts
class MyComponent {
  static myVar = 1;
}
```

as a result, whether or not this static initializer exists or not is
stuffed onto the component's metadata. this allows us to store if an
additional transformation is needed or not after transpilation has
occurred. specifcially, should we add an `export` keyword to a modified
class declaration in the form:
```ts
// determine if this `VariableDeclaration` needs an `export`
const MyComponent = class {}
```

Additional Information

Consider a TypeScript class that:
- has a static member that is defined on the class
- is exported from the file in which it is defined
```ts
export class MyComponent {
  static myVar = 1;
}
```

[In TypeScript v4.9 and lower](https://www.typescriptlang.org/play?ts=4.9.5#code/KYDwDg9gTgLgBAYwDYEMDOa4FkCeBhCAW0gDtgT4BvAKAEg0YUYBLBOQnANRSjgF44ARgDc1AL5A) the class is transpiled as such:
```js
export class MyComponent {
}
MyComponent.myVar = 1;
```

[Starting with TypeScript v5.0](https://www.typescriptlang.org/play?ts=5.0.4#code/KYDwDg9gTgLgBAYwDYEMDOa4FkCeBhCAW0gDtgT4BvAKAEg0YUYBLBOQnANRSjgF44ARgDc1AL5A), the class is transpiled like so:
```js
class MyComponent {
}
MyComponent.myVar = 1;
export { MyComponent };
```
Where the `export` occurs separate from the class's declaration:
```diff
- export class MyComponent {
+ class MyComponent {
}
MyComponent.myVar = 1;
+ export { MyComponent };
```

Note this only occurs when both conditions above are met. The following
code snippet compiles the same using TypeScript [v4.9](https://www.typescriptlang.org/play?ts=4.9.5#code/KYDwDg9gTgLgBAYwDYEMDOa4FkCeBhCAW0gDtgT4BvAKAF8g) and [v5.0](https://www.typescriptlang.org/play?ts=5.0.4&ssl=2&ssc=2&pln=1&pc=1#code/KYDwDg9gTgLgBAYwDYEMDOa4FkCeBhCAW0gDtgT4BvAKAF8g):
```ts
export class MyComponent {
}
// transpiles to
export class MyComponent {
}
```

The difference in compilation when both conditions are met leads to a bug in
Stencil v3.3.0, the first version of the library which supports TypeScript v5.0.

Stencil made the assumption that the `export` keyword continues to be inlined
with the class declaration, rather than a new `ExportDeclaration` instance
being created. In Stencil v3.3.0, the `export` keyword is added to the generated
code:
```ts
export class MyComponent {};
MyComponent.myVar = 1;
export { MyComponent }; // 2 exports causes an error!
```
Two exports of the class (`MyComponent`) causes errors following the TypeScript
compilation step in Stencil:
```console
[ ERROR ]  Rollup: Parse Error: <FILE_PATH>
		   Duplicate export 'MyComponent' (Note that you need plugins to
		   import files that are not JavaScript) Error parsing: <FILE_PATH>
```

In an ideal world, we delegate inserting `export` to the TypeScript compiler.
However, this is not possible as the `ExportDeclaration` is _only_ created for
classes that are exported and have static members. So while removing Stencil's
`export` keyword injection will work for:
```ts
export class MyComponent {
  static myVar = 1;
}
// becomes the following on TS 5.0 if we remove Stencil's `export` injection
class MyComponent {
}
MyComponent.myVar = 1;
export { MyComponent };
```
it will not work for:
```ts
export class MyComponent {
  myVar = 1;
}
// becomes the following on TS 5.0 if we remove Stencil's `export` injection
class MyComponent {
  constructor() {
    this.myVar = 1;
  }
}
```
as `export` is removed from `MyComponent` in the compilation process. This behavior is
controlled by the [`TransformOptions#componentExport` field](https://github.com/ionic-team/stencil/blob/bd1023a67c5224cbae3e445170cb13227a5cf8fb/src/declarations/stencil-public-compiler.ts#L2683).

Import injection should only occur when neither of two aforementioned criteria are met:
- has a static member that is defined on the class
- is exported from the file in which it is defined
  • Loading branch information
rwaskiewicz committed Jun 2, 2023
1 parent c83c113 commit 632a06e
Show file tree
Hide file tree
Showing 22 changed files with 450 additions and 18 deletions.
1 change: 1 addition & 0 deletions src/compiler/output-targets/dist-custom-elements/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ export const addCustomElementInputs = (

if (cmp.isPlain) {
exp.push(`export { ${importName} as ${exportName} } from '${cmp.sourceFilePath}';`);
// TODO(STENCIL-855): Invalid syntax generation, note the unbalanced left curly brace
indexExports.push(`export { {${exportName} } from '${coreKey}';`);
} else {
// the `importName` may collide with the `exportName`, alias it just in case it does with `importAs`
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/transformers/component-lazy/lazy-component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export const updateLazyComponentClass = (
cmp: d.ComponentCompilerMeta
) => {
const members = updateLazyComponentMembers(transformOpts, styleStatements, classNode, moduleFile, cmp);
return updateComponentClass(transformOpts, classNode, classNode.heritageClauses, members);
return updateComponentClass(transformOpts, classNode, classNode.heritageClauses, members, moduleFile);
};

const updateLazyComponentMembers = (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export const updateNativeComponentClass = (
): ts.ClassDeclaration | ts.VariableStatement => {
const heritageClauses = updateNativeHostComponentHeritageClauses(classNode, moduleFile);
const members = updateNativeHostComponentMembers(transformOpts, classNode, moduleFile, cmp);
return updateComponentClass(transformOpts, classNode, heritageClauses, members);
return updateComponentClass(transformOpts, classNode, heritageClauses, members, moduleFile);
};

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ export const proxyCustomElement = (
// update the variable statement containing the updated declaration list
const updatedVariableStatement = ts.factory.updateVariableStatement(
stmt,
[ts.factory.createModifier(ts.SyntaxKind.ExportKeyword)],
stmt.modifiers,
updatedDeclarationList
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,14 @@ import { augmentDiagnosticWithNode, buildError } from '@utils';
import ts from 'typescript';

import type * as d from '../../../declarations';
import { retrieveTsDecorators, retrieveTsModifiers } from '../transform-utils';
import {
convertValueToLiteral,
createStaticGetter,
retrieveTsDecorators,
retrieveTsModifiers,
} from '../transform-utils';
import { componentDecoratorToStatic } from './component-decorator';
import { hasStaticInitializerInClass } from './convert-static-members';
import { isDecoratorNamed } from './decorator-utils';
import {
CLASS_DECORATORS_TO_REMOVE,
Expand Down Expand Up @@ -72,6 +78,14 @@ const visitClassDeclaration = (
listenDecoratorsToStatic(diagnostics, decoratedMembers, filteredMethodsAndFields);
}

// Handle static members that are initialized
const hasStaticMembersWithInit = classMembers.some(hasStaticInitializerInClass);
if (hasStaticMembersWithInit) {
filteredMethodsAndFields.push(
createStaticGetter('stencilHasStaticMembersWithInit', convertValueToLiteral(hasStaticMembersWithInit))
);
}

// We call the `handleClassFields` method which handles transforming any
// class fields, removing them from the class and adding statements to the
// class' constructor which instantiate them there instead.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import ts from 'typescript';

/**
* Helper function to detect if a class element fits the following criteria:
* - It is a property declaration (e.g. `foo`)
* - It has an initializer (e.g. `foo *=1*`)
* - The property declaration has the `static` modifier on it (e.g. `*static* foo =1`)
* @param classElm the class member to test
* @returns true if the class member fits the above criteria, false otherwise
*/
export const hasStaticInitializerInClass = (classElm: ts.ClassElement): boolean => {
return (
ts.isPropertyDeclaration(classElm) &&
classElm.initializer !== undefined &&
Array.isArray(classElm.modifiers) &&
classElm.modifiers!.some((modifier) => modifier.kind === ts.SyntaxKind.StaticKeyword)
);
};
2 changes: 2 additions & 0 deletions src/compiler/transformers/remove-static-meta-properties.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export const removeStaticMetaProperties = (classNode: ts.ClassDeclaration): ts.C
});
};

// TODO(STENCIL-856): Move these properties to constants for better type safety within the codebase
/**
* A list of static getter names that are specific to Stencil to exclude from a class's member list
*/
Expand All @@ -37,6 +38,7 @@ const REMOVE_STATIC_GETTERS = new Set([
'methods',
'states',
'originalStyleUrls',
'stencilHasStaticMembersWithInit',
'styleMode',
'style',
'styles',
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/transformers/static-to-meta/component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,14 @@ export const parseStaticComponentMeta = (
const docs = serializeSymbol(typeChecker, symbol);
const isCollectionDependency = moduleFile.isCollectionDependency;
const encapsulation = parseStaticEncapsulation(staticMembers);

const cmp: d.ComponentCompilerMeta = {
tagName: tagName,
excludeFromCollection: moduleFile.excludeFromCollection,
isCollectionDependency,
componentClassName: cmpNode.name ? cmpNode.name.text : '',
elementRef: parseStaticElementRef(staticMembers),
encapsulation,
hasStaticInitializedMember: getStaticValue(staticMembers, 'stencilHasStaticMembersWithInit') ?? false,
shadowDelegatesFocus: parseStaticShadowDelegatesFocus(encapsulation, staticMembers),
properties: parseStaticProps(staticMembers),
virtualProperties: parseVirtualProps(docs),
Expand Down
158 changes: 158 additions & 0 deletions src/compiler/transformers/test/convert-static-members.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
import ts from 'typescript';

import { hasStaticInitializerInClass } from '../decorators-to-static/convert-static-members';

describe('convert-static-members', () => {
describe('hasStaticInitializerInClass', () => {
it('returns true for a static property with an initializer', () => {
const classWithStaticMembers = ts.factory.createClassDeclaration(
[ts.factory.createToken(ts.SyntaxKind.ExportKeyword)],
ts.factory.createIdentifier('ClassWithStaticMember'),
undefined,
undefined,
[
ts.factory.createPropertyDeclaration(
[ts.factory.createToken(ts.SyntaxKind.StaticKeyword)],
ts.factory.createIdentifier('propertyName'),
undefined,
undefined,
ts.factory.createStringLiteral('initial value')
),
]
);
expect(classWithStaticMembers.members.some(hasStaticInitializerInClass)).toBe(true);
});

it('returns true for a private static property with an initializer', () => {
const classWithStaticMembers = ts.factory.createClassDeclaration(
[ts.factory.createToken(ts.SyntaxKind.ExportKeyword)],
ts.factory.createIdentifier('ClassWithStaticMember'),
undefined,
undefined,
[
ts.factory.createPropertyDeclaration(
[ts.factory.createToken(ts.SyntaxKind.PrivateKeyword), ts.factory.createToken(ts.SyntaxKind.StaticKeyword)],
ts.factory.createIdentifier('propertyName'),
undefined,
undefined,
ts.factory.createStringLiteral('initial value')
),
]
);
expect(classWithStaticMembers.members.some(hasStaticInitializerInClass)).toBe(true);
});

it('returns true for a static property with an initializer with multiple members', () => {
const classWithStaticMembers = ts.factory.createClassDeclaration(
[ts.factory.createToken(ts.SyntaxKind.ExportKeyword)],
ts.factory.createIdentifier('ClassWithStaticAndNonStaticMembers'),
undefined,
undefined,
[
ts.factory.createPropertyDeclaration(
undefined,
ts.factory.createIdentifier('nonStaticProperty'),
undefined,
undefined,
ts.factory.createStringLiteral('some value')
),
ts.factory.createPropertyDeclaration(
[ts.factory.createToken(ts.SyntaxKind.StaticKeyword)],
ts.factory.createIdentifier('propertyName'),
undefined,
undefined,
ts.factory.createStringLiteral('initial value')
),
]
);
expect(classWithStaticMembers.members.some(hasStaticInitializerInClass)).toBe(true);
});

it('returns false for a class without any members', () => {
const classWithStaticMembers = ts.factory.createClassDeclaration(
[ts.factory.createToken(ts.SyntaxKind.ExportKeyword)],
ts.factory.createIdentifier('ClassWithNoMembers'),
undefined,
undefined,
[] // no members for this class
);
expect(classWithStaticMembers.members.some(hasStaticInitializerInClass)).toBe(false);
});

it('returns false for a static property without an initializer', () => {
const classWithStaticMembers = ts.factory.createClassDeclaration(
[ts.factory.createToken(ts.SyntaxKind.ExportKeyword)],
ts.factory.createIdentifier('ClassWithUninitializedStaticMember'),
undefined,
undefined,
[
ts.factory.createPropertyDeclaration(
[ts.factory.createToken(ts.SyntaxKind.StaticKeyword)],
ts.factory.createIdentifier('propertyName'),
undefined,
undefined,
undefined // the initializer is false
),
]
);
expect(classWithStaticMembers.members.some(hasStaticInitializerInClass)).toBe(false);
});

it('returns false for a private static property without an initializer', () => {
const classWithStaticMembers = ts.factory.createClassDeclaration(
[ts.factory.createToken(ts.SyntaxKind.ExportKeyword)],
ts.factory.createIdentifier('ClassWithUninitializedStaticMember'),
undefined,
undefined,
[
ts.factory.createPropertyDeclaration(
[ts.factory.createToken(ts.SyntaxKind.PrivateKeyword), ts.factory.createToken(ts.SyntaxKind.StaticKeyword)],
ts.factory.createIdentifier('propertyName'),
undefined,
undefined,
undefined // the initializer is false
),
]
);
expect(classWithStaticMembers.members.some(hasStaticInitializerInClass)).toBe(false);
});

it('returns false for a modified property with an initializer', () => {
const classWithStaticMembers = ts.factory.createClassDeclaration(
[ts.factory.createToken(ts.SyntaxKind.ExportKeyword)],
ts.factory.createIdentifier('ClassWithNonStaticMember'),
undefined,
undefined,
[
ts.factory.createPropertyDeclaration(
[ts.factory.createToken(ts.SyntaxKind.PrivateKeyword)], // the property is declared as private
ts.factory.createIdentifier('propertyName'),
undefined,
undefined,
ts.factory.createStringLiteral('initial value')
),
]
);
expect(classWithStaticMembers.members.some(hasStaticInitializerInClass)).toBe(false);
});

it('returns false for an unmodified property with an initializer', () => {
const classWithStaticMembers = ts.factory.createClassDeclaration(
[ts.factory.createToken(ts.SyntaxKind.ExportKeyword)],
ts.factory.createIdentifier('ClassWithUnmodifiedMembers'),
undefined,
undefined,
[
ts.factory.createPropertyDeclaration(
undefined, // the property declaration has no modifiers
ts.factory.createIdentifier('propertyName'),
undefined,
undefined,
ts.factory.createStringLiteral('initial value')
),
]
);
expect(classWithStaticMembers.members.some(hasStaticInitializerInClass)).toBe(false);
});
});
});
18 changes: 18 additions & 0 deletions src/compiler/transformers/test/parse-members.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { transpileModule } from './transpile';

describe('parse static members', () => {
it('places a static getter on the component', () => {
const t = transpileModule(`
@Component({tag: 'cmp-a'})
export class CmpA {
static myStatic = 'a value';
render() {
return <div>Hello, I have {CmpA.myStatic}</div>
}
}
`);

expect(t.outputText.includes('static get stencilHasStaticMembersWithInit() { return true; }')).toBe(true);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,19 @@ describe('proxy-custom-element-function', () => {
const transformer = proxyCustomElement(compilerCtx, transformOpts);
const transpiledModule = transpileModule(code, null, compilerCtx, [], [transformer]);

expect(formatCode(transpiledModule.outputText)).toContain(
formatCode(
`const ${componentClassName} = /*@__PURE__*/ __stencil_proxyCustomElement(class ${componentClassName} extends HTMLElement {}, true);`
)
);
});

it('wraps an exported class initializer in a proxyCustomElement call', () => {
const code = `export const ${componentClassName} = class extends HTMLElement {};`;

const transformer = proxyCustomElement(compilerCtx, transformOpts);
const transpiledModule = transpileModule(code, null, compilerCtx, [], [transformer]);

expect(formatCode(transpiledModule.outputText)).toContain(
formatCode(
`export const ${componentClassName} = /*@__PURE__*/ __stencil_proxyCustomElement(class ${componentClassName} extends HTMLElement {}, true);`
Expand All @@ -97,7 +110,7 @@ describe('proxy-custom-element-function', () => {

expect(formatCode(transpiledModule.outputText)).toContain(
formatCode(
`export const foo = 'hello world!', ${componentClassName} = /*@__PURE__*/ __stencil_proxyCustomElement(class ${componentClassName} extends HTMLElement {}, true);`
`const foo = 'hello world!', ${componentClassName} = /*@__PURE__*/ __stencil_proxyCustomElement(class ${componentClassName} extends HTMLElement {}, true);`
)
);
});
Expand All @@ -110,7 +123,7 @@ describe('proxy-custom-element-function', () => {

expect(formatCode(transpiledModule.outputText)).toContain(
formatCode(
`export const ${componentClassName} = /*@__PURE__*/ __stencil_proxyCustomElement(class ${componentClassName} extends HTMLElement {}, true), foo = 'hello world!';`
`const ${componentClassName} = /*@__PURE__*/ __stencil_proxyCustomElement(class ${componentClassName} extends HTMLElement {}, true), foo = 'hello world!';`
)
);
});
Expand All @@ -123,7 +136,7 @@ describe('proxy-custom-element-function', () => {

expect(formatCode(transpiledModule.outputText)).toContain(
formatCode(
`export const foo = 'hello world!', ${componentClassName} = /*@__PURE__*/ __stencil_proxyCustomElement(class ${componentClassName} extends HTMLElement {}, true), bar = 'goodbye?';`
`const foo = 'hello world!', ${componentClassName} = /*@__PURE__*/ __stencil_proxyCustomElement(class ${componentClassName} extends HTMLElement {}, true), bar = 'goodbye?';`
)
);
});
Expand Down
14 changes: 11 additions & 3 deletions src/compiler/transformers/test/transpile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -175,11 +175,19 @@ export function transpileModule(
*/
const prettifyTSOutput = (tsOutput: string): string => tsOutput.replace(/\s+/gm, ' ');

export function getStaticGetter(output: string, prop: string) {
const toEvaluate = `return ${output.replace('export', '')}`;
/**
* Helper function for tests that converts stringified JavaScript to a runtime value.
* A value from the generated JavaScript is returned based on the provided property name.
* @param stringifiedJs the stringified JavaScript
* @param propertyName the property name to pull off the generated JavaScript
* @returns the value associated with the provided property name. Returns undefined if an error occurs while converting
* the stringified JS to JavaScript, or if the property does not exist on the generated JavaScript.
*/
export function getStaticGetter(stringifiedJs: string, propertyName: string): string | void {
const toEvaluate = `return ${stringifiedJs.replace('export', '')}`;
try {
const Obj = new Function(toEvaluate);
return Obj()[prop];
return Obj()[propertyName];
} catch (e) {
console.error(e);
console.error(toEvaluate);
Expand Down
Loading

0 comments on commit 632a06e

Please sign in to comment.