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

bug: constructor() method no longer runs for components implementing an interface #3773

Closed
3 tasks done
kdeds opened this issue Oct 26, 2022 · 6 comments
Closed
3 tasks done
Assignees
Labels
Bug: Validated This PR or Issue is verified to be a bug within Stencil

Comments

@kdeds
Copy link

kdeds commented Oct 26, 2022

Prerequisites

Stencil Version

2.19.0

Current Behavior

Our components each implement a common interface. We initialize some properties in the constructor(). This used to work fine, but now in Stencil 2.19 the function does not seem to run.

Here is a simple example. In the code below, this.classProps is undefined in the render method.

import { Component, h} from '@stencil/core';
import { RhclComponent } from "../../model/RhclComponent";

@Component({
  tag: 'rhcl-example',
  styleUrl: 'example.scss',
  shadow: true
})
export class Example implements RhclComponent {

  private classProps: Array<string>;

  constructor() {
    this.classProps = ["variant", "theme"];
  }

  render() {
    console.log(this.classProps); // this is undefined
    return (
      <div>
      </div>
    );
  }
}

Expected Behavior

Code in constructor() would run and this.classProps in the above example would be an array with the entries ["variant", "theme"] .

Steps to Reproduce

  1. Create a component which implements an interface.
  2. Add some code in the constructor().
  3. With Stencil 2.18 installed, verify the code in the constructor runs.
  4. With Stencil 2.19 installed, observe that the code in the constructor does not run.

Code Reproduction URL

https://github.com/kdeds/stencil-constructor-bug

Additional Information

No response

@ionitron-bot ionitron-bot bot added the triage label Oct 26, 2022
@kdeds
Copy link
Author

kdeds commented Oct 26, 2022

Seems to be related to bd340b8

We moved our constructor code into componentWillLoad() which is fine for us but maybe there is something that can be done for backwards compatibility, or documentation to make it clear not to use constructors. This took a while to track down today and was breaking our whole library 😅

@alicewriteswrongs
Copy link
Contributor

Hey @kdeds, I'm going to look into this today. Stencil should allow you to use constructors, so this is a bug which should be fixed. If possible I'd recommend staying on a 2.18.x version for now, no need to go ahead with modifying your component library to be compatible with the bug!

Thanks for providing the reproduction!

@alicewriteswrongs alicewriteswrongs added the Bug: Validated This PR or Issue is verified to be a bug within Stencil label Oct 27, 2022
@ionitron-bot ionitron-bot bot removed the triage label Oct 27, 2022
@alicewriteswrongs alicewriteswrongs self-assigned this Oct 27, 2022
alicewriteswrongs added a commit that referenced this issue Oct 27, 2022
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 f977830 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.
@alicewriteswrongs
Copy link
Contributor

alicewriteswrongs commented Oct 27, 2022

Ah ok I believe I understand what the problem is. There is an explainer here about the change which resulted in this behavior: f977830

In short, to support setting typescript to emit ES2022+ code we need to transpile class fields, like this one:

class MyComponent {
  @Prop() foo: string = "hey!";
}

to the following:

class MyComponent {
  constructor () {
    this.foo = "hey!";
  }
}

A bunch of Stencil's runtime behavior for props and so on is based on using Object.defineProperty to setup getters and setters, and those don't play so well with the new support for class fields as standardized in ES2022, so we need to do that conversion.

Anyhow - I looked through our implementation of that transpilation and there was an oversight where existing statements inside of the constructor method are not preserved in the case where a statement has to be added.

This PR fixes the issue: #3776

rwaskiewicz pushed a commit that referenced this issue Oct 27, 2022
…ors (#3776)

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 f977830 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.
@rwaskiewicz
Copy link
Member

Hey @kdeds 👋

@alicewriteswrongs' fix has been included in a pre-release of Stencil, v2.19.2-0. Can you please install it and verify that it fixes the issue?

@kdeds
Copy link
Author

kdeds commented Oct 27, 2022

Sorry, had some weird issues trying to install it this morning but just gave it another try and got it downloaded. Looks like the fix works! Thank you for the quick turnaround :D
image
image

@rwaskiewicz
Copy link
Member

The fix for this bug has been released as a part of Stencil v2.19.2. As a result, I'm going to close this issue. Should the bug re-appear, please feel free to open a new issue. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug: Validated This PR or Issue is verified to be a bug within Stencil
Projects
None yet
Development

No branches or pull requests

3 participants