fix(babel-plugin-component): avoid unnecessary registerDecorators #2741

nolanlawson · 2022-03-15T18:25:39Z

Details

This makes it so that we don't unnecessarily call registerDecorators on classes that don't extend anything. (This is an effective test because LWC components must extend something – either LightningElement or a superclass/mixin.)

class NotALightningElement {
  foo: 'foo'
}
-lwc.registerDecorators(NotALightningElement, {
-  fields: ["foo"]
-});

Does this pull request introduce a breaking change?

✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

✅ No, it does not introduce an observable change.

There is maybe some very clever way you could detect this if you really wanted to. But I don't think it actually matters in practice.

GUS work item

W-10736098

nolanlawson · 2022-03-15T19:57:22Z

/nucleus test

jye-sf · 2022-03-15T20:02:50Z

packages/@lwc/babel-plugin-component/src/decorators/index.js

@@ -216,6 +216,12 @@ function decorators({ types: t }) {
                return;
            }

+            if (node.superClass === null) {
+                // Components *must* extend from either LightningElement or some other superclass (e.g. a mixin).
+                // We can skip classes without a superclass to avoid adding unnecessary registerDecorators() calls


Just to confirm, we also don't need to call registerDecorators() or perform any of the subsequent logic on LightningElement, mixins, or other "superclasses" right?

(I believe we do have decorators in mixins, but I'm not familiar enough with this area to know whether we need to process the mixin class itself or whether it'll be sufficient to process the class extending/using the mixin)

Both superclasses and mixins may have fields, so we do need to call registerDecorators() on them, yes. For LightningElement no, we don't need to do that.

Luckily both superclasses and mixins extend from another class. I'll add a Karma test for this, though, since I realized we don't seem to have one for mixins.

Great! Just a nitpick then, but my impression from the comment was that superclasses/mixins were similar to LightningElement in being top-level classes with no supers. If everything (components, mixins, etc) that has fields must extend from another class, can we make that clearer?

Ah you're right. Yes we can change this comment.

pmdartus

One of the side-effects of this change is that decorators applied to classes that aren't extending from a superclass will remain in the generated output. Until now all the decorators were transformed. Instead of failing silently at runtime, rollup will fail to parse the generated output:

Error: Unexpected character '@'

Before merging this PR, I would like to make sure we have considered all the ramifications of this change.

nolanlawson · 2022-03-16T18:36:52Z

Thanks @pmdartus. This seems like a question we can answer with static analysis:

Do any classes that don't have superclasses contain decorators like @api, @track, or @wire?

nolanlawson · 2022-03-16T22:52:50Z

W-10858780

pmdartus · 2022-03-17T10:00:23Z

We could be running static analysis against database components to make decorators are applied to classes with a superclass. On top of this, I would like to make sure we have the correct linting in place to catch this error early on.

As mentioned in my previous comment, rollup will report a tokenization error if decorators aren't applied correctly. In terms of developer experience, it would be preferable to report a more meaningful error when the compiler detects an invalid usage of a decorator.

nolanlawson · 2022-03-17T16:00:05Z

In terms of developer experience, it would be preferable to report a more meaningful error when the compiler detects an invalid usage of a decorator.

We could write an ESLint rule. Or were you thinking of just changing the error message in Rollup?

jye-sf · 2022-03-17T18:30:18Z

Will the ESLint rule capture all possible paths, or are there environments where internal/external devs could bypass the ESLint rule? If the latter is at all possible, we may need to have some mechanism to detect the invalid usage (either explicitly detecting the usage during transformation/compilation or implicitly detecting the error message) in addition to the ESLint rule.

pmdartus · 2022-03-18T10:29:12Z

In terms of developer experience, it would be preferable to report a more meaningful error when the compiler detects an invalid usage of a decorator.

We could write an ESLint rule. Or were you thinking of just changing the error message in Rollup?

Yes, I was thinking about creating a new ESLint rule. Today we have valid-api, valid-track and valid-wire. What about no-unknown-decorators?

Will the ESLint rule capture all possible paths, or are there environments where internal/external devs could bypass the ESLint rule?

ESLint should be able to capture all the possible usages of JavaScript decorators. As always internal developers could bypass it by using an eslint disablement mechanism. In which case, they are on their own.

nolanlawson · 2022-03-21T16:24:08Z

Yes, I was thinking about creating a new ESLint rule.

Filed W-10875815 to capture this.

nolanlawson · 2022-05-06T22:20:19Z

@pmdartus

One of the side-effects of this change is that decorators applied to classes that aren't extending from a superclass will remain in the generated output. Until now all the decorators were transformed. Instead of failing silently at runtime, rollup will fail to parse the generated output

After looking into this, I'm not sure this is actually a concern. It looks like, today, if you apply @api or @track to a non-LightningElement, it causes a runtime error in the browser (demo):

Uncaught TypeError: undefined is not a VM.
    at assertIsVM (main.js:6366:11)
    at getAssociatedVM (main.js:6378:5)
    at NotAnLwcComponent.set [as foo] (main.js:2762:18)
    at new NotAnLwcComponent (main.js:8132:14)
    at main.js:8145:13

So it seems that users are already prevented from applying decorators to non-LightningElements. This PR would just change a runtime error into a compile-time error. Maybe this is still a problem, but it seems like less of a problem than we previously thought.

pmdartus · 2022-05-12T09:41:59Z

I agree with your previous statement. Running the example you shared with this change results in the following error:

src/modules/x/app/app.js
SyntaxError: /Users/p.dartus/code/github/nolanlawson/lwc-barebone/src/modules/x/app/app.js: Decorators are not enabled.
If you are using ["@babel/plugin-proposal-decorators", { "legacy": true }], make sure it comes *before* "@babel/plugin-proposal-class-properties" and enable loose mode, like so:
	["@babel/plugin-proposal-decorators", { "legacy": true }]
	["@babel/plugin-proposal-class-properties", { "loose": true }]
  1 | import { LightningElement, api } from 'lwc';
  2 |
> 3 | class NotAnLwcComponent {
    | ^
  4 |   @api foo = 'foo'
  5 | }
  6 |
    at File.buildCodeFrameError (/Users/p.dartus/code/github/nolanlawson/lwc-barebone/node_modules/@babel/core/lib/transformation/file/file.js:249:12)
    at NodePath.buildCodeFrameError (/Users/p.dartus/code/github/nolanlawson/lwc-barebone/node_modules/@babel/traverse/lib/path/index.js:143:21)
    at shouldTransform (/Users/p.dartus/code/github/nolanlawson/lwc-barebone/node_modules/@babel/helper-create-class-features-plugin/lib/features.js:127:16)
    at PluginPass.Class (/Users/p.dartus/code/github/nolanlawson/lwc-barebone/node_modules/@babel/helper-create-class-features-plugin/lib/index.js:90:44)
    at newFn (/Users/p.dartus/code/github/nolanlawson/lwc-barebone/node_modules/@babel/traverse/lib/visitors.js:177:21)
    at NodePath._call (/Users/p.dartus/code/github/nolanlawson/lwc-barebone/node_modules/@babel/traverse/lib/path/context.js:53:20)
    at NodePath.call (/Users/p.dartus/code/github/nolanlawson/lwc-barebone/node_modules/@babel/traverse/lib/path/context.js:40:17)
    at NodePath.visit (/Users/p.dartus/code/github/nolanlawson/lwc-barebone/node_modules/@babel/traverse/lib/path/context.js:100:31)
    at TraversalContext.visitQueue (/Users/p.dartus/code/github/nolanlawson/lwc-barebone/node_modules/@babel/traverse/lib/context.js:103:16)
    at TraversalContext.visitMultiple (/Users/p.dartus/code/github/nolanlawson/lwc-barebone/node_modules/@babel/traverse/lib/context.js:72:17)

If we can produce a better error than this, I would be more than happy to merge this PR.

nolanlawson · 2022-05-12T16:49:17Z

packages/@lwc/compiler/src/transformers/__tests__/transform-javascript.spec.ts

+    if (!errored) {
+        fail('Expected an error to be thrown');
+    }
+});


@pmdartus Added a more helpful error message ^

How it looks in the console:

[!] (plugin rollup-plugin-lwc-compiler) Error: SyntaxError: LWC1150: Decorators like @api, @track, and @wire are only supported in LightningElement classes. /Users/nlawson/workspace/lwc-barebone/src/modules/x/app/app.js: Decorators are not enabled. If you are using ["@babel/plugin-proposal-decorators", { "legacy": true }], make sure it comes *before* "@babel/plugin-proposal-class-properties" and enable loose mode, like so: ["@babel/plugin-proposal-decorators", { "legacy": true }] ["@babel/plugin-proposal-class-properties", { "loose": true }] 1 | import { LightningElement, api } from 'lwc'; 2 | > 3 | class NotAnLwcComponent { | ^ 4 | @api foo = 'foo' 5 | } 6 | src/modules/x/app/app.js SyntaxError: /Users/nlawson/workspace/lwc-barebone/src/modules/x/app/app.js: Decorators are not enabled. If you are using ["@babel/plugin-proposal-decorators", { "legacy": true }], make sure it comes *before* "@babel/plugin-proposal-class-properties" and enable loose mode, like so: ["@babel/plugin-proposal-decorators", { "legacy": true }] ["@babel/plugin-proposal-class-properties", { "loose": true }] 1 | import { LightningElement, api } from 'lwc'; 2 | > 3 | class NotAnLwcComponent { | ^ 4 | @api foo = 'foo' 5 | } 6 | at File.buildCodeFrameError (/Users/nlawson/workspace/lwc/node_modules/@babel/core/lib/transformation/file/file.js:249:12) at NodePath.buildCodeFrameError (/Users/nlawson/workspace/lwc/node_modules/@babel/core/node_modules/@babel/traverse/lib/path/index.js:143:21) at shouldTransform (/Users/nlawson/workspace/lwc/node_modules/@babel/helper-create-class-features-plugin/lib/features.js:127:16) at PluginPass.Class (/Users/nlawson/workspace/lwc/node_modules/@babel/helper-create-class-features-plugin/lib/index.js:92:44) at newFn (/Users/nlawson/workspace/lwc/node_modules/@babel/core/node_modules/@babel/traverse/lib/visitors.js:177:21) at NodePath._call (/Users/nlawson/workspace/lwc/node_modules/@babel/core/node_modules/@babel/traverse/lib/path/context.js:53:20) at NodePath.call (/Users/nlawson/workspace/lwc/node_modules/@babel/core/node_modules/@babel/traverse/lib/path/context.js:40:17) at NodePath.visit (/Users/nlawson/workspace/lwc/node_modules/@babel/core/node_modules/@babel/traverse/lib/path/context.js:100:31) at TraversalContext.visitQueue (/Users/nlawson/workspace/lwc/node_modules/@babel/core/node_modules/@babel/traverse/lib/context.js:103:16) at TraversalContext.visitMultiple (/Users/nlawson/workspace/lwc/node_modules/@babel/core/node_modules/@babel/traverse/lib/context.js:72:17)

nolanlawson · 2022-05-24T18:55:58Z

After static analysis, the risk/reward ratio seems too high to make this worthwhile.

jye-sf reviewed Mar 15, 2022

View reviewed changes

jye-sf approved these changes Mar 15, 2022

View reviewed changes

nolanlawson force-pushed the nolan/no-register-decorators branch from d585620 to 6a96e20 Compare March 15, 2022 23:50

pmdartus reviewed Mar 16, 2022

View reviewed changes

nolanlawson added the nomerge label Mar 16, 2022

nolanlawson force-pushed the nolan/no-register-decorators branch from 6a96e20 to 3d7ae32 Compare May 6, 2022 22:13

nolanlawson added 4 commits May 12, 2022 09:48

fix(babel-plugin-component): avoid unnecessary registerDecorators

067c80e

test: add test for @api and mixins

027b27b

fix: fix comment

a8336e1

fix: add a more helpful error message

56fe5e0

nolanlawson force-pushed the nolan/no-register-decorators branch from 0778132 to 56fe5e0 Compare May 12, 2022 16:48

nolanlawson commented May 12, 2022

View reviewed changes

nolanlawson requested a review from pmdartus May 12, 2022 17:17

pmdartus approved these changes May 16, 2022

View reviewed changes

ravijayaramappa approved these changes May 16, 2022

View reviewed changes

nolanlawson closed this May 24, 2022

nolanlawson deleted the nolan/no-register-decorators branch July 25, 2022 16:20

nolanlawson mentioned this pull request Feb 18, 2023

feat(versioning): remove registerDecorators for non-LightningElement classes #3354

Closed

nolanlawson mentioned this pull request Aug 7, 2023

fix(babel-plugin-component): skip unnecessary registerDecorators (BREAKING CHANGE) #3660

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(babel-plugin-component): avoid unnecessary registerDecorators #2741

fix(babel-plugin-component): avoid unnecessary registerDecorators #2741

nolanlawson commented Mar 15, 2022 •

edited

Loading

nolanlawson commented Mar 15, 2022

jye-sf Mar 15, 2022

jye-sf Mar 15, 2022

nolanlawson Mar 15, 2022

jye-sf Mar 15, 2022

nolanlawson Mar 15, 2022 •

edited

Loading

pmdartus left a comment

nolanlawson commented Mar 16, 2022

nolanlawson commented Mar 16, 2022

pmdartus commented Mar 17, 2022

nolanlawson commented Mar 17, 2022

jye-sf commented Mar 17, 2022

pmdartus commented Mar 18, 2022

nolanlawson commented Mar 21, 2022

nolanlawson commented May 6, 2022

pmdartus commented May 12, 2022

nolanlawson May 12, 2022 •

edited

Loading

nolanlawson commented May 24, 2022

fix(babel-plugin-component): avoid unnecessary registerDecorators #2741

fix(babel-plugin-component): avoid unnecessary registerDecorators #2741

Conversation

nolanlawson commented Mar 15, 2022 • edited Loading

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

GUS work item

nolanlawson commented Mar 15, 2022

jye-sf Mar 15, 2022

Choose a reason for hiding this comment

jye-sf Mar 15, 2022

Choose a reason for hiding this comment

nolanlawson Mar 15, 2022

Choose a reason for hiding this comment

jye-sf Mar 15, 2022

Choose a reason for hiding this comment

nolanlawson Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

pmdartus left a comment

Choose a reason for hiding this comment

nolanlawson commented Mar 16, 2022

nolanlawson commented Mar 16, 2022

pmdartus commented Mar 17, 2022

nolanlawson commented Mar 17, 2022

jye-sf commented Mar 17, 2022

pmdartus commented Mar 18, 2022

nolanlawson commented Mar 21, 2022

nolanlawson commented May 6, 2022

pmdartus commented May 12, 2022

nolanlawson May 12, 2022 • edited Loading

Choose a reason for hiding this comment

nolanlawson commented May 24, 2022

nolanlawson commented Mar 15, 2022 •

edited

Loading

nolanlawson Mar 15, 2022 •

edited

Loading

nolanlawson May 12, 2022 •

edited

Loading