feat(babel-plugin-transform-lwc-class): Transform imported decorators #49

pmdartus · 2018-01-31T01:04:25Z

Details

Currently, the compiler assumes that decorators like api, track and wire are globally available. This PR updates the compiler to only transform decorators that are imported by from engine.

A component previously written like ...

import { Element } from 'engine';

export default class Test extends Element {
	@track state = {};
}

... should be updated into.

import { Element, track } from 'engine';

export default class Test extends Element {
	@track state = {};
}

Does this PR introduce a breaking change?

Yes
No

If yes, please describe the impact and migration path for existing applications:

The compiler will now throw if it find a decorator which is not imported from engine. A codemod has been developed on the side to help developers migrate to the new syntax.

salesforce-best-lwc-internal · 2018-01-31T01:11:31Z

Benchmark comparison

Base commit: 29cb560 | Target commit: cce129e

benchmark	base(`29cb560`)	target(`cce129e`)	trend

yungcheng · 2018-01-31T04:58:53Z

packages/babel-plugin-transform-lwc-class/src/__tests__/decorators.test.js

+
+    transformTest('throws if a decorator is dereferenced', `
+        import { track } from 'engine';
+        const trok = track;


const trock = track;

yungcheng · 2018-01-31T05:00:31Z

packages/babel-plugin-transform-lwc-class/src/__tests__/decorators.test.js

+        }
+    `, {
+        error: {
+            message: 'test.js: "track" can only be used as a class decorator',


the error message is confusing, can we say something regarding the fact that track has been referenced by trock and that's not supported?

This one is tricky to implement. There are many ways to use track we can't create customize messages for each cases.

yungcheng · 2018-01-31T05:04:33Z

packages/babel-plugin-transform-lwc-class/src/__tests__/wire-decorator.test.js

    });

-    test('decorator expects an oject as second parameter', `
+    transformTest('decorator expects an oject as second parameter', `


as @kevinv11n would notice, oject is missing a b

yungcheng · 2018-01-31T05:07:30Z

packages/babel-plugin-transform-lwc-class/src/__tests__/wire-decorator.test.js

    });

-    test('throws when using 2 wired decorators', `
+    transformTest('throws when using 2 wired decorators', `
+        import { wire } from 'engine';
        export default class Test {
            @wire('record', { recordId: '$recordId', fields: ['Address'] })
            @wire('record', { recordId: '$recordId', fields: ['Name'] })
            wiredWithTrack


nitpick - wiredWithTrack is a bad name for this test case

yungcheng

some minor changes are needed

yungcheng · 2018-01-31T05:22:22Z

packages/babel-plugin-transform-lwc-class/src/__tests__/component.test.js

+                class Test1 extends Element {}
+
+                export default class Test2 extends Element {
+                render() {


nitpick, fix the indention for render() {...}

yungcheng · 2018-01-31T05:31:10Z

packages/babel-plugin-transform-lwc-class/src/__tests__/wire-decorator.test.js

@@ -1,162 +1,188 @@
-const test = require('./utils/test-transform').test(
+const transformTest = require('./utils/test-transform').transformTest(


i actually think it would be less confusing if you do
const test = require('./utils/test-transform').transformTest(require('../index'));

test is really transformTester, yet you name it transformTest so when I read between util/test-transform.js and this test file, i got confused.

just my 2 cents, i am okay if you don't want to change it

salesforce-best-lwc-internal · 2018-01-31T21:10:06Z

Benchmark comparison

Base commit: 29cb560 | Target commit: b7dbb4b

benchmark	base(`29cb560`)	target(`b7dbb4b`)	trend

caridy · 2018-02-01T16:30:05Z

should we wait for @diervo to finish the work on decorators in general to make this change?

pmdartus · 2018-02-01T17:16:42Z

What is the decorator work @diervo is working on?

yungcheng · 2018-02-01T18:16:09Z

are you referring to @context? I think that's independent to this PR (it might even benefit from this PR implementation wise)

caridy · 2018-02-01T18:33:39Z

I don't know, maybe we should chat with @diervo. My understand is that we could move out wire and context once he finishes the changes.

pmdartus added 20 commits January 4, 2018 16:28

API decorator

5da9c69

Fix all the tests

0a0205e

Use single file instead of several visitors

e851de3

Migrate all the decorators to the new infra structure

6458a02

Add back labels

0f288d9

Update tests

4349fa0

Fix all the integration tests

a538449

Fix benchmark imports

9aa80b9

Move constants missing class properties

4613eb8

Fix wire service tests

a75194b

Merge branch 'master' into pmdartus/imported-decorators

8ae026d

Rename Raptor to LWC

7dd9bef

Change decorator visitor function name

de25709

Fix misc typos

4df5196

Store in decorator data structure field type

f0fdf6c

Merge branch 'master' into pmdartus/imported-decorators

0413648

Fix review bug

83346fd

Merge branch 'master' into pmdartus/imported-decorators

1f3a7d9

Restrict usage of other decorators in the compiler

338cf90

Fix transform tester

cce129e

pmdartus requested review from yungcheng and apapko January 31, 2018 01:04

yungcheng reviewed Jan 31, 2018

View reviewed changes

yungcheng suggested changes Jan 31, 2018

View reviewed changes

yungcheng reviewed Jan 31, 2018

View reviewed changes

pmdartus added 2 commits January 31, 2018 12:59

Rename test to pluginTest for readability purposes

f144eba

Fix nitpick

b7dbb4b

yungcheng approved these changes Feb 1, 2018

View reviewed changes

pmdartus merged commit 172c75b into master Feb 2, 2018

pmdartus deleted the pmdartus/imported-decorators branch February 2, 2018 19:12

nolanlawson mentioned this pull request Aug 8, 2022

Remove part restriction #2057

Open

pmdartus pushed a commit that referenced this pull request Nov 12, 2024

rfc: mixed shadow mode (#49)

a9059ef

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(babel-plugin-transform-lwc-class): Transform imported decorators #49

feat(babel-plugin-transform-lwc-class): Transform imported decorators #49

pmdartus commented Jan 31, 2018

salesforce-best-lwc-internal bot commented Jan 31, 2018

yungcheng Jan 31, 2018

yungcheng Jan 31, 2018

pmdartus Jan 31, 2018

yungcheng Jan 31, 2018

yungcheng Jan 31, 2018

yungcheng left a comment •

edited

Loading

yungcheng Jan 31, 2018

yungcheng Jan 31, 2018 •

edited

Loading

salesforce-best-lwc-internal bot commented Jan 31, 2018

caridy commented Feb 1, 2018

pmdartus commented Feb 1, 2018

yungcheng commented Feb 1, 2018

caridy commented Feb 1, 2018

		@@ -1,162 +1,188 @@
		const test = require('./utils/test-transform').test(
		const transformTest = require('./utils/test-transform').transformTest(

feat(babel-plugin-transform-lwc-class): Transform imported decorators #49

feat(babel-plugin-transform-lwc-class): Transform imported decorators #49

Conversation

pmdartus commented Jan 31, 2018

Details

Does this PR introduce a breaking change?

salesforce-best-lwc-internal bot commented Jan 31, 2018

Benchmark comparison

yungcheng Jan 31, 2018

Choose a reason for hiding this comment

yungcheng Jan 31, 2018

Choose a reason for hiding this comment

pmdartus Jan 31, 2018

Choose a reason for hiding this comment

yungcheng Jan 31, 2018

Choose a reason for hiding this comment

yungcheng Jan 31, 2018

Choose a reason for hiding this comment

yungcheng left a comment • edited Loading

Choose a reason for hiding this comment

yungcheng Jan 31, 2018

Choose a reason for hiding this comment

yungcheng Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

salesforce-best-lwc-internal bot commented Jan 31, 2018

Benchmark comparison

caridy commented Feb 1, 2018

pmdartus commented Feb 1, 2018

yungcheng commented Feb 1, 2018

caridy commented Feb 1, 2018

yungcheng left a comment •

edited

Loading

yungcheng Jan 31, 2018 •

edited

Loading