fix: runtime decorators (#6076)

* fix: make decorators great again * chore: WIP. Pretty much there * chore: pretty much there.. sans tests * chore: tidying up * chore: make it work properly again * chore: first unit test plus edge smoothing * chore: pretty much dun * chore: clearer jsdoc * chore: tidy * chore: formatting --------- Co-authored-by: John Jenkins <[email protected]>
ionic-team · Jan 2, 2025 · 9e6483a · 9e6483a
1 parent 5d29255
commit 9e6483a
Show file tree

Hide file tree

Showing 30 changed files with 680 additions and 266 deletions.
diff --git a/.gitignore b/.gitignore
@@ -59,3 +59,4 @@ test/wdio/test-components-no-external-runtime
 test/wdio/www-global-script/
 test/wdio/www-prerender-script
 test/wdio/www-invisible-prehydration/
+test/wdio/test-ts-target-output
diff --git a/src/client/client-host-ref.ts b/src/client/client-host-ref.ts
@@ -1,4 +1,5 @@
 import { BUILD } from '@app-data';
+import { MEMBER_FLAGS } from '@utils/constants';
 
 import type * as d from '../declarations';
 
@@ -35,10 +36,13 @@ export const getHostRef = (ref: d.RuntimeRef): d.HostRef | undefined => hostRefs
  *
  * @param lazyInstance the lazy instance of interest
  * @param hostRef that instances `HostRef` object
- * @returns a reference to the host ref WeakMap
  */
-export const registerInstance = (lazyInstance: any, hostRef: d.HostRef) =>
+export const registerInstance = (lazyInstance: any, hostRef: d.HostRef) => {
   hostRefs.set((hostRef.$lazyInstance$ = lazyInstance), hostRef);
+  if (BUILD.modernPropertyDecls && (BUILD.state || BUILD.prop)) {
+    reWireGetterSetter(lazyInstance, hostRef);
+  }
+};
 
 /**
  * Register a host element for a Stencil component, setting up various metadata
@@ -67,7 +71,77 @@ export const registerHost = (hostElement: d.HostElement, cmpMeta: d.ComponentRun
     hostElement['s-p'] = [];
     hostElement['s-rc'] = [];
   }
-  return hostRefs.set(hostElement, hostRef);
+
+  const ref = hostRefs.set(hostElement, hostRef);
+
+  if (!BUILD.lazyLoad && BUILD.modernPropertyDecls && (BUILD.state || BUILD.prop)) {
+    reWireGetterSetter(hostElement, hostRef);
+  }
+
+  return ref;
 };
 
 export const isMemberInElement = (elm: any, memberName: string) => memberName in elm;
+
+/**
+ * - Re-wires component prototype `get` / `set` with instance `@State` / `@Prop` decorated fields.
+ * - Makes sure the initial value from the `Element` is synced to the instance `@Prop` decorated fields.
+ *
+ * Background:
+ * During component init, Stencil loops through any `@Prop()` or `@State()` decorated properties
+ * and sets up getters and setters for each (within `src/runtime/proxy-component.ts`) on a component prototype.
+ *
+ * These accessors sync-up class instances with their `Element` and controls re-renders.
+ * With modern JS, compiled classes (e.g. `target: 'es2022'`) compiled Stencil components went from:
+ *
+ * ```ts
+ * class MyComponent {
+ *   constructor() {
+ *     this.prop1 = 'value1';
+ *   }
+ * }
+ * ```
+ * To:
+ * ```ts
+ * class MyComponent {
+ *  prop1 = 'value2';
+ *  // ^^ These override the accessors originally set on the prototype
+ * }
+ * ```
+ *
+ * @param instance - class instance to re-wire
+ * @param hostRef - component reference meta
+ */
+const reWireGetterSetter = (instance: any, hostRef: d.HostRef) => {
+  const cmpMeta = hostRef.$cmpMeta$;
+  const members = Object.entries(cmpMeta.$members$ ?? {});
+
+  members.map(([memberName, [memberFlags]]) => {
+    if (
+      BUILD.state &&
+      BUILD.prop &&
+      (memberFlags & MEMBER_FLAGS.Getter) === 0 &&
+      (memberFlags & MEMBER_FLAGS.Prop || memberFlags & MEMBER_FLAGS.State)
+    ) {
+      const ogValue = instance[memberName];
+
+      // Get the original Stencil prototype `get` / `set`
+      const lazyDescriptor = Object.getOwnPropertyDescriptor(Object.getPrototypeOf(instance), memberName);
+
+      // Re-wire original accessors to the new instance
+      Object.defineProperty(instance, memberName, {
+        get() {
+          return lazyDescriptor.get.call(this);
+        },
+        set(newValue) {
+          lazyDescriptor.set.call(this, newValue);
+        },
+        configurable: true,
+        enumerable: true,
+      });
+      instance[memberName] = hostRef.$instanceValues$.has(memberName)
+        ? hostRef.$instanceValues$.get(memberName)
+        : ogValue;
+    }
+  });
+};
diff --git a/src/compiler/app-core/app-data.ts b/src/compiler/app-core/app-data.ts
@@ -50,6 +50,7 @@ export const getBuildFeatures = (cmps: ComponentCompilerMeta[]): BuildFeatures =
     member: cmps.some((c) => c.hasMember),
     method: cmps.some((c) => c.hasMethod),
     mode: cmps.some((c) => c.hasMode),
+    modernPropertyDecls: cmps.some((c) => c.hasModernPropertyDecls),
     observeAttribute: cmps.some((c) => c.hasAttribute || c.hasWatchCallback),
     prop: cmps.some((c) => c.hasProp),
     propBoolean: cmps.some((c) => c.hasPropBoolean),

diff --git a/src/compiler/output-targets/dist-lazy/lazy-build-conditionals.ts b/src/compiler/output-targets/dist-lazy/lazy-build-conditionals.ts
@@ -18,6 +18,7 @@ export const getLazyBuildConditionals = (
 
   const hasHydrateOutputTargets = config.outputTargets.some(isOutputTargetHydrate);
   build.hydrateClientSide = hasHydrateOutputTargets;
+  build.modernPropertyDecls = cmps.some((c) => c.hasModernPropertyDecls);
 
   updateBuildConditionals(config, build);
 

diff --git a/src/compiler/transformers/decorators-to-static/attach-internals.ts b/src/compiler/transformers/decorators-to-static/attach-internals.ts
@@ -2,12 +2,7 @@ import { buildError } from '@utils';
 import ts from 'typescript';
 
 import type * as d from '../../../declarations';
-import {
-  convertValueToLiteral,
-  createStaticGetter,
-  retrieveTsDecorators,
-  tsPropDeclNameAsString,
-} from '../transform-utils';
+import { convertValueToLiteral, createStaticGetter, retrieveTsDecorators, tsPropDeclName } from '../transform-utils';
 import { isDecoratorNamed } from './decorator-utils';
 
 /**
@@ -56,7 +51,7 @@ export const attachInternalsDecoratorsToStatic = (
 
   const [decoratedProp] = attachInternalsMembers;
 
-  const name = tsPropDeclNameAsString(decoratedProp, typeChecker);
+  const { staticName: name } = tsPropDeclName(decoratedProp, typeChecker);
 
   newMembers.push(createStaticGetter('attachInternalsMemberName', convertValueToLiteral(name)));
 };
diff --git a/src/compiler/transformers/decorators-to-static/convert-decorators.ts b/src/compiler/transformers/decorators-to-static/convert-decorators.ts
@@ -2,20 +2,11 @@ import { augmentDiagnosticWithNode, buildError } from '@utils';
 import ts from 'typescript';
 
 import type * as d from '../../../declarations';
-import {
-  retrieveTsDecorators,
-  retrieveTsModifiers,
-  tsPropDeclNameAsString,
-  updateConstructor,
-} from '../transform-utils';
+import { retrieveTsDecorators, retrieveTsModifiers, updateConstructor } from '../transform-utils';
 import { attachInternalsDecoratorsToStatic } from './attach-internals';
 import { componentDecoratorToStatic } from './component-decorator';
 import { isDecoratorNamed } from './decorator-utils';
-import {
-  CLASS_DECORATORS_TO_REMOVE,
-  CONSTRUCTOR_DEFINED_MEMBER_DECORATORS,
-  MEMBER_DECORATORS_TO_REMOVE,
-} from './decorators-constants';
+import { CLASS_DECORATORS_TO_REMOVE, MEMBER_DECORATORS_TO_REMOVE } from './decorators-constants';
 import { elementDecoratorsToStatic } from './element-decorator';
 import { eventDecoratorsToStatic } from './event-decorator';
 import { ImportAliasMap } from './import-alias-map';
@@ -163,14 +154,11 @@ const visitClassDeclaration = (
     );
   }
 
-  // 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.
-  const updatedClassFields = handleClassFields(classNode, filteredMethodsAndFields, typeChecker, importAliasMap);
-
   validateMethods(diagnostics, classMembers);
 
   const currentDecorators = retrieveTsDecorators(classNode);
+  const updatedClassFields: ts.ClassElement[] = updateConstructor(classNode, filteredMethodsAndFields, []);
+
   return ts.factory.updateClassDeclaration(
     classNode,
     [
@@ -232,7 +220,7 @@ const removeStencilMethodDecorators = (
       } else if (ts.isGetAccessor(member)) {
         return ts.factory.updateGetAccessorDeclaration(
           member,
-          ts.canHaveModifiers(member) ? ts.getModifiers(member) : undefined,
+          [...(newDecorators ?? []), ...(retrieveTsModifiers(member) ?? [])],
           member.name,
           member.parameters,
           member.type,
@@ -243,25 +231,15 @@ const removeStencilMethodDecorators = (
         err.messageText = 'A get accessor should be decorated before a set accessor';
         augmentDiagnosticWithNode(err, member);
       } else if (ts.isPropertyDeclaration(member)) {
-        if (shouldInitializeInConstructor(member, importAliasMap)) {
-          // 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 {
-          // update the property to remove decorators
-          const modifiers = retrieveTsModifiers(member);
-          return ts.factory.updatePropertyDeclaration(
-            member,
-            [...(newDecorators ?? []), ...(modifiers ?? [])],
-            member.name,
-            member.questionToken,
-            member.type,
-            member.initializer,
-          );
-        }
+        const modifiers = retrieveTsModifiers(member);
+        return ts.factory.updatePropertyDeclaration(
+          member,
+          [...(newDecorators ?? []), ...(modifiers ?? [])],
+          member.name,
+          member.questionToken,
+          member.type,
+          member.initializer,
+        );
       } else {
         const err = buildError(diagnostics);
         err.messageText = 'Unknown class member encountered!';
@@ -309,168 +287,3 @@ export const filterDecorators = (
   // return the node's original decorators, or undefined
   return decorators;
 };
-
-/**
- * This updates a Stencil component class declaration AST node to handle any
- * class fields with Stencil-specific decorators (`@State`, `@Prop`, etc). For
- * reasons explained below, we need to remove these fields from the class and
- * add code to the class's constructor to instantiate them manually.
- *
- * When a class field is decorated with a Stencil-defined decorator, we rely on
- * defining our own setters and getters (using `Object.defineProperty`) to
- * implement the behavior we want. Unfortunately, in ES2022 and newer versions
- * of the EcmaScript standard the behavior for class fields like the following
- * is incompatible with using manually-defined getters and setters:
- *
- * ```ts
- * class MyClass {
- *   foo = "bar"
- * }
- * ```
- *
- * In ES2022+ if we try to use `Object.defineProperty` on this class's
- * prototype in order to define a `set` and `get` function for the
- * property `foo` it will not override the default behavior of the
- * instance field `foo`, so doing something like the following:
- *
- * ```ts
- * Object.defineProperty(MyClass.prototype, "foo", {
- *   get() {
- *     return "Foo is: " + this.foo
- *   }
- * });
- * ```
- *
- * and then calling `myClassInstance.foo` will _not_ return `"Foo is: bar"` but
- * just `"bar"`. This is because the standard ECMAScript behavior is now to use
- * the internals of `Object.defineProperty` on a class instance to instantiate
- * fields, and that call at instantiation-time overrides what's set on the
- * prototype. For details, see the accepted ECMAScript proposal for this
- * behavior:
- *
- * https://github.com/tc39/proposal-class-fields#public-fields-created-with-objectdefineproperty
- *
- * Why is this important? With `target` set to an ECMAScript version prior to
- * ES2022 TypeScript by default would emit a class which instantiated the field
- * in its constructor, something like this:
- *
- * ```ts
- * class CompiledMyClass {
- *   constructor() {
- *     this.foo = "bar"
- *   }
- * }
- * ```
- *
- * This plays nicely with later using `Object.defineProperty` on the prototype
- * to define getters and setters, or simply with defining them right on the
- * class (see the code in `proxyComponent`, `proxyCustomElement`, and friends).
- *
- * However, with a `target` of ES2022 or higher (e.g. `ESNext`) default
- * behavior for TypeScript is instead to emit code like this:
- *
- * ```ts
- * class CompiledMyClass {
- *   foo = "bar"
- * }
- * ```
- *
- * This output is more correct because the compiled code 1) more closely
- * resembles the TypeScript source and 2) is using standard JS syntax instead
- * of desugaring it. There is an announcement in the release notes for
- * TypeScript v3.7 which explains some helpful background about the change,
- * and about the `useDefineForClassFields` TypeScript option which lets you
- * opt-in to the old output:
- *
- * https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#the-usedefineforclassfields-flag-and-the-declare-property-modifier
- *
- * For our use-case, however, the ES2022+ behavior doesn't work, since we need
- * to be able to define getters and setters on these fields. We could require
- * that the TypeScript configuration used for Stencil have the
- * `useDefineForClassFields` setting set to `false`, but that would have the
- * undesirable side-effect that class fields which are _not_
- * decorated with a Stencil decorator would also be instantiated in the
- * constructor.
- *
- * So instead, we take matters into our own hands. When we encounter a class
- * field which is decorated with a Stencil decorator we remove it from the
- * class and add a statement to the constructor to instantiate it with the
- * correct default value.
- *
- * **Note**: this function will modify a constructor if one is already present on
- * the class or define a new one otherwise.
- *
- * @param classNode a TypeScript AST node for a Stencil component class
- * @param classMembers the class members that we need to update
- * @param typeChecker a reference to the {@link ts.TypeChecker}
- * @param importAliasMap a map of Stencil decorator names to their import names
- * @returns a list of updated class elements which can be inserted into the class
- */
-function handleClassFields(
-  classNode: ts.ClassDeclaration,
-  classMembers: ts.ClassElement[],
-  typeChecker: ts.TypeChecker,
-  importAliasMap: ImportAliasMap,
-): ts.ClassElement[] {
-  const statements: ts.ExpressionStatement[] = [];
-  const updatedClassMembers: ts.ClassElement[] = [];
-
-  for (const member of classMembers) {
-    if (shouldInitializeInConstructor(member, importAliasMap) && ts.isPropertyDeclaration(member)) {
-      const memberName = tsPropDeclNameAsString(member, typeChecker);
-
-      // 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 {
-      // if it's not a class field that is decorated with a Stencil decorator then
-      // we just push it onto our class member list
-      updatedClassMembers.push(member);
-    }
-  }
-
-  if (statements.length === 0) {
-    // we didn't encounter any class fields we need to update, so we can
-    // just return the list of class members (no need to create an empty
-    // constructor)
-    return updatedClassMembers;
-  } else {
-    // create or update a constructor which contains the initializing statements
-    // we created above
-    return updateConstructor(classNode, updatedClassMembers, statements);
-  }
-}
-
-/**
- * Check whether a given class element should be rewritten from a class field
- * to a constructor-initialized value. This is basically the case for fields
- * decorated with `@Prop` and `@State`. See {@link handleClassFields} for more
- * details.
- *
- * @param member the member to check
- * @param importAliasMap a map of Stencil decorator names to their import names
- * @returns whether this should be rewritten or not
- */
-const shouldInitializeInConstructor = (member: ts.ClassElement, importAliasMap: ImportAliasMap): boolean => {
-  const currentDecorators = retrieveTsDecorators(member);
-  if (currentDecorators === undefined) {
-    // decorators have already been removed from this element, indicating that
-    // we don't need to do anything
-    return false;
-  }
-  const filteredDecorators = filterDecorators(
-    currentDecorators,
-    CONSTRUCTOR_DEFINED_MEMBER_DECORATORS.map((decorator) => importAliasMap.get(decorator)),
-  );
-  return currentDecorators !== filteredDecorators;
-};