bug: custom decorators are not invoked for state properties

[jgroth]

Prerequisites

• I have read the Contributing Guidelines.
• I agree to follow the Code of Conduct.
• I have searched for existing issues that already report this problem, without success.

Stencil Version

2.19.0

Current Behavior

If a property is decorated with both @State and another decorator, the code for the other decorator will never be invoked.

In the following component nothing will be logged to the console with Stencil 2.19.0. It works perfectly in 2.18.1. Probably introduced in #3614

const MyDecorator: () => PropertyDecorator = () => {
  return (target: Object, propertyKey: string) => {
    console.log({target, propertyKey});
  };
};

@Component({
  tag: 'my-component',
  shadow: true,
})
export class MyComponent {
  @State()
  @MyDecorator()
  decoratedProp: any;
}

After inspecting the compiled javascript files, the TypeScript __decorate helper function is missing for the component in 2.19.0, and is present in 2.18.1

Expected Behavior

Decorators added to properties also decorated with @State should be invoked

Steps to Reproduce

Create a new component with a property decorated with both @State and something else. The other decorator will not work

Code Reproduction URL

https://github.com/jgroth/stencil-decorator-bug

Additional Information

No response

[alicewriteswrongs]

Hello @jgroth, thanks for filing the issue and for providing a reproduction case! I just checked this out and was able to confirm that this is an issue, custom decorators worked great in 2.18.x but the 2.19.0 release broke them because of this change: #3614

In short, in order to support emitting ES2022+ we needed to change some parts of how we support the built-in Stencil decorators, and unfortunately due to some oversight on our part that change stepped on custom decorator support.

This is a problem we plan to fix, and something the team is actively looking into. I'm going to label this so we can ingest it into our internal backlog and prioritize it going forward.

Thanks again for filing the issue, and stay tuned for updates soon!

[jgroth]

Thanks for the quick reply! Any estimate on a timeline when it might be fixed?
This is a quite serious breaking change for us and will prevent us from upgrading. We use this quite extensively to reduce the amount of boilerplate we have to write when connecting a component to a Redux state, e.g.

class MyComponent {
    @State()
    @SelectCurrentUser()
    private currentUser: User;
}

[alicewriteswrongs]

@jgroth ah sorry that it broke a common pattern you all are using. Part of the sticky bit with this issue is that we need to do some discovery work to figure out exactly the right way to address this — this doesn't mean that it will take eons to address, but just that at present I don't have a clear roadmap of exactly how we'll fix it in my head, so I can't commit to a specific timeline at present.

However! That said, the ticket for doing this work has been pulled in to our next sprint, so we will start work on it relatively soon.

[Dayvisson]

Hello @alicewriteswrongs, thanks for your attention in this subject.
Does you have any updates about this issue?
We are looking foward to use stencil 3.0, but this issue is preventing us from upgrading too.

[alicewriteswrongs]

Hey @Dayvisson, thanks for following up on this and keeping the fire burning here. We're currently doing the discovery / research work I mentioned about about how Stencil should handle decorators going forward. The recent inclusion of ECMAScript standard-compliant decorators in TypeScript 5.0 has increased the urgency of addressing this, as we need to determine how we can 1) support upgrading to TypeScript 5.0 2) support Stencil's current built-in decorator behavior and 3) support user-defined custom decorators.

The wrinkle is that Stencil's built-in decorators are essentially implemented using custom TypeScript transformers (transformations on the TypeScript syntax tree that run during the compilation process) and they aren't directly compatible with either the decorators that TS developers have been able to use for a few years with the experimentalDecorators flag or with ECMAScript decorators as documented in the current TC39 proposal.

So we're evaluating what to do here. Once we have a firmer idea of what we think the right path forward is here I will make sure to get your feedback on it, as we do want to make sure that using custom decorators is supported in Stencil in a way that jibes with the community's existing workflows.

Thanks again for pinging, and apologies that this has been open a while!

[jgroth]

Any updates on this? 🙂

[alicewriteswrongs]

Hey @jgroth unfortunately I don't have anything specific to share at present as far as a timeline for addressing this or more on our specific thinking on how to allow for custom decorators in Stencil going forward. At present the loose plan is to refactor how Stencil's current decorators are implemented by the Stencil compiler, basically re-implementing them as standards-compliant decorators which can be used without any custom compiler support (or possibly with more minimal support than is needed right now). That rough idea hasn't really been proven out yet though, so definitely subject to change.

Right now I can also say we're close to clearing a few big things off of our plates which will free us up to look at the decorator issue more closely in general. I know it's been a while without any updates, but we do still want to support custom decorators!

[jgroth]

This is honestly a mess. I was going to try and see if we could update our code to use the new decorator syntax from TypeScript 5, but that does not seem to be supported either. The code does compile even if VSCode gives a lot of errors and complains about the Stencil decorators when I remove experimentalDecorators from tsconfig, but the decorator code is not executing.

[louis-bompart]

The issues impacts both @Prop and @State and the root cause (IMHO) come from here:

stencil/src/compiler/transformers/decorators-to-static/convert-decorators.ts

Lines 356 to 372 in a1ab21b

	if (shouldInitializeInConstructor(member) && ts.isPropertyDeclaration(member)) {
	const memberName = tsPropDeclNameAsString(member);
	// this is a class field that we'll need to handle, so lets push a statement for
	// initializing the value onto our statements list
	statements.push(
	ts.factory.createExpressionStatement(
	ts.factory.createBinaryExpression(
	ts.factory.createPropertyAccessExpression(ts.factory.createThis(), ts.factory.createIdentifier(memberName)),
	ts.factory.createToken(ts.SyntaxKind.EqualsToken),
	// if the member has no initializer we should default to setting it to
	// just 'undefined'
	member.initializer ?? ts.factory.createIdentifier('undefined'),
	),
	),
	);
	} else {

And

stencil/src/compiler/transformers/decorators-to-static/convert-decorators.ts

Lines 190 to 197 in a1ab21b

	if (shouldInitializeInConstructor(member)) {
	// if the current class member is decorated with either 'State' or
	// 'Prop' we need to modify the property declaration to transform it
	// from a class field but we handle this in the `handleClassFields`
	// method below, so we just want to return the class member here
	// untouched.
	return member;
	} else {

Essentially, when Stencil found out a Prop or State decorator, it removes it from the class members declarations, and instead declare it in the constructor.

This has the adverse effect of removing all decorators, including the customs one.

I think removeStencilMethodDecorators should not do any distinctions and wipe the decorators of the classMembers without distinction. So essentially, always do this for properties:

stencil/src/compiler/transformers/decorators-to-static/convert-decorators.ts

Lines 199 to 207 in a1ab21b

	const modifiers = retrieveTsModifiers(member);
	return ts.factory.updatePropertyDeclaration(
	member,
	[...(newDecorators ?? []), ...(modifiers ?? [])],
	member.name,
	member.questionToken,
	member.type,
	member.initializer,
	);

)

After that, instead of passing the filteredMembers to handleClassFields give both the filteredMembers and the untouched ones to it. With the untouched ones, handleClassFIelds generates the statements it needs. It does not generate the updatedClassMembers anymore and instead use the filteredMembers list direcly.

I'll open a PR later today/this week to proposes theses changes after running more tests

[alicewriteswrongs]

Hey @louis-bompart, thanks for investigating this a bit more! It's a bit of a tricky situation with decorators in Stencil at present.

One of the limitations that we have here which makes the situation for custom decorators unfortunate at the moment is that for fields which are decorated with @State, @Prop, and the like we currently implement only part of their behavior at compile-time using TypeScript transformers and the rest is implemented at runtime using Object.defineProperty. I think the way to think about this is basically that the compile-time part of implementing the behavior for these decorators is basically writing down static information about the decorated properties (default values and whatnot) and then the runtime part is actually creating a function and defining a property on the component.

That runtime bit happens here in the proxyComponent function:

stencil/src/runtime/proxy-component.ts

Lines 32 to 83 in 9a92ad1

	members.map(([memberName, [memberFlags]]) => {
	if (
	(BUILD.prop || BUILD.state) &&
	(memberFlags & MEMBER_FLAGS.Prop ||
	((!BUILD.lazyLoad || flags & PROXY_FLAGS.proxyState) && memberFlags & MEMBER_FLAGS.State))
	) {
	// proxyComponent - prop
	Object.defineProperty(prototype, memberName, {
	get(this: d.RuntimeRef) {
	// proxyComponent, get value
	return getValue(this, memberName);
	},
	set(this: d.RuntimeRef, newValue) {
	// only during dev time
	if (BUILD.isDev) {
	const ref = getHostRef(this);
	if (
	// we are proxying the instance (not element)
	(flags & PROXY_FLAGS.isElementConstructor) === 0 &&
	// the element is not constructing
	(ref.$flags$ & HOST_FLAGS.isConstructingInstance) === 0 &&
	// the member is a prop
	(memberFlags & MEMBER_FLAGS.Prop) !== 0 &&
	// the member is not mutable
	(memberFlags & MEMBER_FLAGS.Mutable) === 0
	) {
	consoleDevWarn(
	`@Prop() "${memberName}" on <${cmpMeta.$tagName$}> is immutable but was modified from within the component.\nMore information: https://stenciljs.com/docs/properties#prop-mutability`,
	);
	}
	}
	// proxyComponent, set value
	setValue(this, memberName, newValue, cmpMeta);
	},
	configurable: true,
	enumerable: true,
	});
	} else if (
	BUILD.lazyLoad &&
	BUILD.method &&
	flags & PROXY_FLAGS.isElementConstructor &&
	memberFlags & MEMBER_FLAGS.Method
	) {
	// proxyComponent - method
	Object.defineProperty(prototype, memberName, {
	value(this: d.HostElement, ...args: any[]) {
	const ref = getHostRef(this);
	return ref.$onInstancePromise$.then(() => ref.$lazyInstance$[memberName](...args));
	},
	});
	}
	});

You can see that the function loops through the component's members and uses Object.defineProperty to setup getters and setters on the class's prototype which call out to the getValue and setValue functions which do a bunch of the work to actually implement the behavior that you want for e.g. @State and @Prop and whatnot.

The problem here is that if you want to use Object.defineProperty to define a property foo on the prototype of a class then properties declared on the class declaration itself end up "winning" at runtime and override those values.

You can try this for yourself by running the following code:

const defineFoo = (klass) => {
    Object.defineProperty(klass.prototype, "foo", {
        get() {
            return "defined by `Object.defineProperty`!"
        }
    })
}

class WithProp {
    foo;
}

class WithoutProp {}

defineFoo(WithProp);
defineFoo(WithoutProp);

let withProp = new WithProp();
let withoutProp = new WithoutProp();

console.log("withProp:", withProp.foo);
console.log("withoutProp:", withoutProp.foo);

Because the WithProp class declaration includes a property foo the Object.defineProperty call on WithProp.prototype with a property name of "foo" doesn't "stick", and at runtime withProp.foo will be undefined while withoutProp.foo will be "defined by `Object.defineProperty`".

This unfortunately means we cannot allow props like foo (in the example above) to stick around on the emitted JS if they are decorated with @State, @Prop, etc, because if those properties are set on the class declaration then we'll be unable to correctly implement the behavior we need via calling Object.defineProperty to define properties on the class' prototype.

I know that's kind of a lot so let me know if that doesn't make sense! But that's why, pending a larger refactor, we unfortunately at present cannot support custom decorators and why the change you proposed ends up breaking the @Prop and @State decorators (I built your PR locally and installed it in a new project I created with npm init stencil and I confirmed that @Prop is broken).

The team does want to fix this and we are planning to do an overhaul of how Stencil's decorators are implemented to get them to a place where they can happily coexist with custom user-defined decorators but we just aren't there yet. I know this isn't a great answer to hear, but that's the state of the project right now.

Hopefully that all helps clarify some of what's going on here, and why for now we unfortunately need to remove fields from the class declaration if they are decorated with a Stencil decorator.

[louis-bompart]

Hey @alicewriteswrongs thanks for the detailed answer, that's much appreciated.
I think I understand what you mean here, the only thing that leaves me confused is that empirically, I'm not able to see which behaviors of @Prop or @State are affected by my proposed changes.

I replicated your test with a patch-package here https://github.com/louis-bompart/stencil-components-decorated, and it seems to me that @Prop is working I think. (just ran npm run dev, does the npm run build output change that drastically?)

Moreover, I know we have an extensive E2E test suite in our component library, and if the tests pass with this patch, it may indicate we have a gap in our tests, and I would very much like to patch this hole 😅

[alicewriteswrongs]

aha well as always I did forget one detail! In order to get TypeScript to emit fields like

class Foo {
  foo = "bar";
}

You need to have the target set to ES2022 or higher and have the useDefineForClassFields typescript compiler option set to true. You can check that out here: https://www.typescriptlang.org/play?useDefineForClassFields=true&target=9#code/MYGwhgzhAEBiD29oG8CwAoa0BmjoF5oAiSAE2yIG4MBfIA

With those two options, if we leave @State and @Prop decorated members on the class it will break the behavior of those props

[louis-bompart]

aha well as always I did forget one detail! In order to get TypeScript to emit fields like

class Foo {
  foo = "bar";
}

You need to have the target set to ES2022 or higher and have the useDefineForClassFields typescript compiler option set to true. You can check that out here: https://www.typescriptlang.org/play?useDefineForClassFields=true&target=9#code/MYGwhgzhAEBiD29oG8CwAoa0BmjoF5oAiSAE2yIG4MBfIA

With those two options, if we leave @State and @Prop decorated members on the class it will break the behavior of those props

Ah, gotcha thanks! I reproduced the issue ✅

Now, pretty sure you must have seen it coming but then, assuming we want to fix the custom decorators without embarking on the big overhaul, how about disallowing this typescript option? That would not be a first I reckon, and would alleviate the issues about custom decorators.
If that's not a viable option, I'd suggest having some branching/options next in the visitDeclaration method to allow either the uses of useDefineForClassFields or a new Stencil option allowCustomDecorators (or only rely on the useDefineForClassFields and warn/throw if we detect a non-stencil decorators)

What do you think?

[alicewriteswrongs]

Those are indeed viable options for getting custom decorators to work again in Stencil given where the project is at now. The Stencil team has evaluated both options (disallowing useDefineForClassFields and adding some branching in the transformer code that handles decorators) and we don't think either of them are the right way forward for custom decorators in Stencil at present. The former restricts Stencil users from using TypeScript compiler options that they could already be using at present without issues, and the latter is sort of a patch which will mean taking on more tech debt in order to get a short-term fix for using custom decorators but which we'll need to undo in the future anyway.

Right now we think the "right way" is to migrate Stencil's decorators over from the compile-time constructs they are right now to be actual, standards-compliant decorator functions exported by the @stencil/core module, a change which will allow Stencil decorators to be handled just like any other decorators in TypeScript.

Now that ECMAScript decorators as standardized by the TC39 are being implemented in TypeScript we have the pieces in place to do this work as far as Stencil's external dependencies are concerned, and further investigation and experimentation in this regard is on the team's roadmap. At present I can't share an exact timeline for when that will be, but this problem is a priority for us and something we definitely, 100% plan to fix.

I know that's disappointing if you're trying to use custom decorators right now in a Stencil project, but we're in a bit of a difficult spot right now. Custom decorators used to work in Stencil somewhat by accident until TypeScript started emitting different code with useDefineForClassFields and target: "es2022" or `target: "esnext" (see #3130 and #4528 for context). Until we can get the work done to migrate Stencil's decorators to standards-compliant decorators we won't be able to restore that functionality.

[louis-bompart]

Thanks for the transparency on the question, that's really appreciated. I understand the decision, it's definitely the best for the long term 💯.

One final thing, can we expect the move from the experimental decorator to the TC39 decorator to be part of a major release of Stencil?
The reason is that, notwithstanding this issue, custom experimental decorators do work, as long as there not mixed with stencil experimental decorators.
I'm assuming that when the migration of the current Stencil decorators towards ECMA decorators is released, the experimentalDecorators: false option in TypeScript will be enforced, breaking current implementations (which is fine, if known).

[alicewriteswrongs]

Yeah I think you're right on that point, we'll need to basically convert over 100% because supporting, in an ongoing way, a mix of transformer-implemented decorators (what we have now) and what I call "TypeScript decorators" (basically decorator functions as enabled by the experimentalDecorators option in TypeScript) and ECMA-compliant decorators really will be a mess...so a change like that will probably end up having to be a major release. We've done two major releases in the last year, so this is something that the team has an appetite for. If we need to make a change for the good of the project we will — Stencil is a codebase which (in JS terms) has been around for a while, and the surface area of the browser that it targets (custom elements, web components) has changed a lot in that time so we have a lot of features which were done in an earlier epoch (if you will) of browser support, community adoption, and tooling development that we want to re-think.

[TheCelavi]

I am experimenting with integration of Preact signals into Stenciljs, just for fun: https://github.com/RunOpenCode/stencil-signal

This could be interesting if allowed:

@Prop() 
@Signaling() 
public prop: string = 'foo';

Why? Well, I could just modify prop and set get/set for it to write value into signal. Than, this signal can be detected as dependency of computed and effect under the hood, in example:

@Prop() 
@Signaling() 
public prop1: string = 'foo';

@Prop() 
@Signaling() 
public prop2: string = 'bar';

private combined: Signal<string> = computed((): string => {
   return `${this.prop1} ${this.prop2}`;
})

Or even better, if used standard TC39 decorator - I could expand its behaviour and have only one decorator for signaling prop.

[louis-bompart]

Hi, @alicewriteswrongs 👋 I noticed that some Issues and PR are starting to get tagged with the Stencil v5 label.
Would it be possible to consider this issue for Stencil v5?

To chime in on what kind of decorators to support for the upcoming versions, ECMA-compliant decorators are not yet the way to go (sadly). While the https://github.com/tc39/proposal-decorators progressed in February, it didn't reach Stage 4. Even if it were to achieve it shortly, it would take several years for the feature to be considered Widely Available by Baseline standard.

So the three choices left, I think, are:

1. Transformer-based Decorators: Stencil could expose API for developers to create their own "Stencil Decorators." (It's been a while since I delved into Stencil code, so I cannot vouch for the feasibility of such a venture).
2. TypeScript experimental decorators have been around for years. It's safe to say they are the de facto standard when people in the TypeScript/JavaScript community talk about or want to use decorators. The challenge would be to redo/rewire Stencil's decorators to use or be compatible with the TS experimentalDecorators
3. To officially not support user-defined decorators with Stencil: Document this limitation and possibly reject user-defined decorators at the compilation time.

Even if I dislike the last option, I think this issue should strive toward a conclusion and not live on after the release of Stencil v5. The gain in insight for the users is still a boon and would prevent user errors.

[maxpatiiuk]

ECMA-compliant decorators are not yet the way to go (sadly). Even if it were to achieve it shortly, it would take several years for the feature to be considered

Sorry if I am missing something, but how is that an issue for a compiler-based framework like Stencil? TypeScript already supports the new decorators standart and can emit code with the necessary polyfills:

https://devblogs.microsoft.com/typescript/announcing-typescript-5-0/#decorators:~:text=Differences%20with%20Experimental%20Legacy%20Decorators

If Stencil is to add support for decorators, it should be the new spec, rather than the older, experimental decorators that, while had some DX advantages, may be deprecated in a future TS version.

[louis-bompart]

ECMA-compliant decorators are not yet the way to go (sadly). Even if it were to achieve it shortly, it would take several years for the feature to be considered

Sorry if I am missing something, but how is that an issue for a compiler-based framework like Stencil? TypeScript already supports the new decorators standart and can emit code with the necessary polyfills:

https://devblogs.microsoft.com/typescript/announcing-typescript-5-0/#decorators:~:text=Differences%20with%20Experimental%20Legacy%20Decorators

If Stencil is to add support for decorators, it should be the new spec, rather than the older, experimental decorators that, while had some DX advantages, may be deprecated in a future TS version.

Both approaches have their pros and cons. One of the cons I see with the new spec is that it is sadly not mature yet, as the recent and substantial changes show tc39/proposal-decorators@c39254c...a81149f.
Moreover, it's interesting to consider what Lit does & says about decorators: https://lit.dev/docs/components/decorators/#decorator-versions. Interestingly, experimentalDecorators are recommended over ECMA ones because the polyfill would increase the compiler output, which would be to consider.

However, recent developments happened on this spec's implementation side. For example, the Chromium bug has been assigned on February 7th, and some prototyping was done last month.

I don't have a strong opinion on any options. However, it would be healthy to have a decision taken on the matter, even the 'no support' one, for the upcoming major release, as it 'solves' how Stencil's users should do things moving forward in the medium or even long term.

[alicewriteswrongs]

@louis-bompart @maxpatiiuk definitely agree it is a good idea to resolve a way forward of some kind for this issue. While we aren't ready to put something out about it yet the team has been doing a good deal of internal investigation and experimentation on this front. We will have something to share soon!

[jensgustafsson]

We have thousands of unique web applications that rely heavily on custom decorators. Upgrading to the latest version of Stencil is a priority for us due to its new features and improvements. However, the lack of support for custom decorators is a significant blocker.

Could you please share your plans regarding custom decorators? Will support for them be reintroduced in future releases? Alternatively, should we consider other solutions and start working towards removing our decorators, which we would prefer to avoid as they greatly simplify our applications?