From 244ea4b0d7d4e959ee1300578de59a3f7f9250ae Mon Sep 17 00:00:00 2001 From: Alice Pote Date: Thu, 27 Oct 2022 10:34:13 -0400 Subject: [PATCH 1/3] fix(compiler): account for an existing constructor in convert-decorators This change ensures that statements in an existing constructor are not thrown on the floor in the case that we need to edit a constructor (i.e. when there is a field with `@Prop` that we need to initialize in the constructor). In f97783029274f9ee5ea58ba74ab15905c5113c93 we made a change to initialize any class fields decorated with `@Prop()` in a constructor. The code to do this would look for a constructor on the class and, if found, update the body of the constructor with statements to initialize the field. Unfortunately, that commit would drop all existing statements in the constructor on the floor! This broke how some Stencil users initialize fields or do certain side effects, since no code they wrote in their constructors would make it through to the built output. This commit fixes the issue by instead setting the constructor body to be all of our newly created statements followed by any existing statements. This will allow users to initialize fields to custom values in the constructor if they so chose. See #3773 for an issue describing the issue. --- .../convert-decorators.ts | 20 +++- .../test/convert-decorators.spec.ts | 91 ++++++++++++++++++- 2 files changed, 106 insertions(+), 5 deletions(-) diff --git a/src/compiler/transformers/decorators-to-static/convert-decorators.ts b/src/compiler/transformers/decorators-to-static/convert-decorators.ts index d6ecc41e05c..77736bfd079 100644 --- a/src/compiler/transformers/decorators-to-static/convert-decorators.ts +++ b/src/compiler/transformers/decorators-to-static/convert-decorators.ts @@ -372,10 +372,22 @@ export const updateConstructor = ( const constructorMethod = classMembers[constructorIndex]; if (constructorIndex >= 0 && ts.isConstructorDeclaration(constructorMethod)) { - const hasSuper = constructorMethod.body.statements.some((s) => s.kind === ts.SyntaxKind.SuperKeyword); + const hasSuper = (constructorMethod.body?.statements ?? []).some((s) => s.kind === ts.SyntaxKind.SuperKeyword); if (!hasSuper && needsSuper(classNode)) { - statements = [createConstructorBodyWithSuper(), ...statements]; + // if there is no super and it needs one the statements comprising the + // body of the constructor should be: + // + // 1. the `super()` call + // 2. the new statements we've created to initialize fields + // 3. the statements currently comprising the body of the constructor + statements = [createConstructorBodyWithSuper(), ...statements, ...constructorMethod.body?.statements ?? []]; + } else { + // if no super is needed then the body of the constructor should be: + // + // 1. the new statements we've created to initialize fields + // 2. the statements currently comprising the body of the constructor + statements = [...statements, ...constructorMethod.body?.statements ?? []]; } classMembers[constructorIndex] = ts.factory.updateConstructorDeclaration( @@ -383,7 +395,7 @@ export const updateConstructor = ( constructorMethod.decorators, constructorMethod.modifiers, constructorMethod.parameters, - ts.factory.updateBlock(constructorMethod.body, statements) + ts.factory.updateBlock(constructorMethod?.body ?? ts.factory.createBlock([]), statements) ); } else { // we don't seem to have a constructor, so let's create one and stick it @@ -396,7 +408,7 @@ export const updateConstructor = ( ts.factory.createConstructorDeclaration( undefined, undefined, - undefined, + [], ts.factory.createBlock(statements, true) ), ...classMembers, diff --git a/src/compiler/transformers/test/convert-decorators.spec.ts b/src/compiler/transformers/test/convert-decorators.spec.ts index db4bde6d8fa..fcee8f557eb 100644 --- a/src/compiler/transformers/test/convert-decorators.spec.ts +++ b/src/compiler/transformers/test/convert-decorators.spec.ts @@ -149,7 +149,96 @@ describe('convert-decorators', () => { ); }); - it('should not add a super call to the constructor if necessary', () => { + it('should preserve statements in an existing constructor', () => { + const t = transpileModule(` + @Component({ + tag: 'my-component', + }) + export class MyComponent { + constructor() { + console.log('boop'); + } + }`); + + expect(t.outputText).toBe( + c`export class MyComponent { + constructor() { + console.log('boop'); + } + + static get is() { + return "my-component"; + }}` + ); + }); + + it('should preserve statements in an existing constructor w/ @Prop', () => { + const t = transpileModule(` + @Component({ + tag: 'my-component', + }) + export class MyComponent { + @Prop() count: number; + + constructor() { + console.log('boop'); + } + }`); + + expect(t.outputText).toContain( + c`constructor() { + this.count = undefined; + console.log('boop'); + }` + ); + }); + + it('should allow user to initialize field in an existing constructor w/ @Prop', () => { + const t = transpileModule(` + @Component({ + tag: 'my-component', + }) + export class MyComponent { + @Prop() count: number; + + constructor() { + this.count = 3; + } + }`); + + // the initialization we do to `undefined` (since no value is present) + // should be before the user's `this.count = 3` to ensure that their code + // wins. + expect(t.outputText).toContain( + c`constructor() { + this.count = undefined; + this.count = 3; + }` + ); + }); + + it('should preserve statements in an existing constructor w/ non-decorated field', () => { + const t = transpileModule(` + @Component({ + tag: 'example', + }) + export class Example implements RhclComponent { + private classProps: Array; + + constructor() { + this.classProps = ["variant", "theme"]; + } + }`); + + expect(t.outputText).toBe( + c`export class Example { + constructor() { + this.classProps = ["variant", "theme"]; + }}` + ); + }); + + it('should not add a super call to the constructor if not necessary', () => { const t = transpileModule(` @Component({tag: 'cmp-a'}) export class CmpA implements Foobar { From 715031af9a9e27946047753b66f867e1bc2ec9e6 Mon Sep 17 00:00:00 2001 From: Alice Pote Date: Thu, 27 Oct 2022 10:55:31 -0400 Subject: [PATCH 2/3] prettier --- .../decorators-to-static/convert-decorators.ts | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/compiler/transformers/decorators-to-static/convert-decorators.ts b/src/compiler/transformers/decorators-to-static/convert-decorators.ts index 77736bfd079..7f4eb6681da 100644 --- a/src/compiler/transformers/decorators-to-static/convert-decorators.ts +++ b/src/compiler/transformers/decorators-to-static/convert-decorators.ts @@ -381,13 +381,13 @@ export const updateConstructor = ( // 1. the `super()` call // 2. the new statements we've created to initialize fields // 3. the statements currently comprising the body of the constructor - statements = [createConstructorBodyWithSuper(), ...statements, ...constructorMethod.body?.statements ?? []]; + statements = [createConstructorBodyWithSuper(), ...statements, ...(constructorMethod.body?.statements ?? [])]; } else { // if no super is needed then the body of the constructor should be: // // 1. the new statements we've created to initialize fields // 2. the statements currently comprising the body of the constructor - statements = [...statements, ...constructorMethod.body?.statements ?? []]; + statements = [...statements, ...(constructorMethod.body?.statements ?? [])]; } classMembers[constructorIndex] = ts.factory.updateConstructorDeclaration( @@ -405,12 +405,7 @@ export const updateConstructor = ( } classMembers = [ - ts.factory.createConstructorDeclaration( - undefined, - undefined, - [], - ts.factory.createBlock(statements, true) - ), + ts.factory.createConstructorDeclaration(undefined, undefined, [], ts.factory.createBlock(statements, true)), ...classMembers, ]; } From 3ab56386821d7c72b6ba79749a9f158513657a07 Mon Sep 17 00:00:00 2001 From: Alice Pote Date: Thu, 27 Oct 2022 11:19:11 -0400 Subject: [PATCH 3/3] review feedback --- .../convert-decorators.ts | 8 ++++--- .../test/convert-decorators.spec.ts | 24 ++++++++++++++++++- 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/src/compiler/transformers/decorators-to-static/convert-decorators.ts b/src/compiler/transformers/decorators-to-static/convert-decorators.ts index 7f4eb6681da..dd22df45a3b 100644 --- a/src/compiler/transformers/decorators-to-static/convert-decorators.ts +++ b/src/compiler/transformers/decorators-to-static/convert-decorators.ts @@ -372,7 +372,9 @@ export const updateConstructor = ( const constructorMethod = classMembers[constructorIndex]; if (constructorIndex >= 0 && ts.isConstructorDeclaration(constructorMethod)) { - const hasSuper = (constructorMethod.body?.statements ?? []).some((s) => s.kind === ts.SyntaxKind.SuperKeyword); + const constructorBodyStatements: ts.NodeArray = + constructorMethod.body?.statements ?? ts.factory.createNodeArray(); + const hasSuper = constructorBodyStatements.some((s) => s.kind === ts.SyntaxKind.SuperKeyword); if (!hasSuper && needsSuper(classNode)) { // if there is no super and it needs one the statements comprising the @@ -381,13 +383,13 @@ export const updateConstructor = ( // 1. the `super()` call // 2. the new statements we've created to initialize fields // 3. the statements currently comprising the body of the constructor - statements = [createConstructorBodyWithSuper(), ...statements, ...(constructorMethod.body?.statements ?? [])]; + statements = [createConstructorBodyWithSuper(), ...statements, ...constructorBodyStatements]; } else { // if no super is needed then the body of the constructor should be: // // 1. the new statements we've created to initialize fields // 2. the statements currently comprising the body of the constructor - statements = [...statements, ...(constructorMethod.body?.statements ?? [])]; + statements = [...statements, ...constructorBodyStatements]; } classMembers[constructorIndex] = ts.factory.updateConstructorDeclaration( diff --git a/src/compiler/transformers/test/convert-decorators.spec.ts b/src/compiler/transformers/test/convert-decorators.spec.ts index fcee8f557eb..39fc080ac63 100644 --- a/src/compiler/transformers/test/convert-decorators.spec.ts +++ b/src/compiler/transformers/test/convert-decorators.spec.ts @@ -222,7 +222,7 @@ describe('convert-decorators', () => { @Component({ tag: 'example', }) - export class Example implements RhclComponent { + export class Example implements FooBar { private classProps: Array; constructor() { @@ -238,6 +238,28 @@ describe('convert-decorators', () => { ); }); + it('should preserve statements in an existing constructor super, decorated field', () => { + const t = transpileModule(` + @Component({ + tag: 'example', + }) + export class Example extends Parent { + @Prop() foo: string = "bar"; + + constructor() { + console.log("hello!") + } + }`); + + expect(t.outputText).toContain( + c`constructor() { + super(); + this.foo = "bar"; + console.log("hello!"); + }` + ); + }); + it('should not add a super call to the constructor if not necessary', () => { const t = transpileModule(` @Component({tag: 'cmp-a'})