feat: allow computed properties in @wire if they are constants or primitives @W-14785085 #3955

wjhsf · 2024-01-18T22:09:39Z

Details

We currently allow computed properties in the @wire config object, in order to allow symbols as property keys. The current code, as written, assumes that a property key is an identifier or a string literal. However, that is not necessarily always the case (e.g. { 123: 'NumericLiteral' }, { [true]: 'BooleanLiteral' }). This PR loosens that restriction to also allow numeric, null, boolean, and bigint literals. Regular expression literals are excluded because they are not primitives, and template literals are excluded because they may contain expressions, meaning they are not guaranteed to always result in the same value (e.g. `${Math.random()}`).

Additionally, this PR restricts variables used in computed properties to only allow constants. This helps prevent accidentally using a computed value that changes, which would prevent reactivity from functioning appropriately.

Fixes #3910.

Does this pull request introduce a breaking change?

🚨 Yes, it does introduce a breaking change.

Does this pull request introduce an observable change?

✅ No, it does not introduce an observable change.
⚠️ Yes, it does include an observable change.

GUS work item

W-14785085

duplicate of decorator-accepts-a-member-expression

wjhsf · 2024-01-19T20:44:55Z

Writing up a follow up ticket, I had a better idea that could solve this issue more robustly. #3956 (comment)

wjhsf · 2024-01-19T21:41:03Z

packages/@lwc/babel-plugin-component/src/decorators/wire/validate.ts

+        validateWireId(id, path, state);
+        if (config) validateWireConfig(config, path, state);
+    } else {
+        // Single argument: should just be id


As far as I can tell, this never actually occurs (the code prior to this change doesn't handle this case and would have thrown). I don't actually know whether it's true that a single argument would be the ID here, but it seems a reasonable guess.

The docs are not clear, but it does seem that the config is optional. A quick search for @wire in SourceGraph reveals several instances of @wire being used with just the single argument. We should probably add a test for this.

Also you are correct, this line is not hit per code coverage.

wjhsf · 2024-01-19T21:43:01Z

packages/@lwc/babel-plugin-component/src/decorators/wire/transform.ts

+                        const value = t.isNullLiteral(p.key) ? null : p.key.value;
+                        return t.stringLiteral(String(value));
+                    }
+                    // If it's not an identifier or primitive literal then it's a computed expression


This fall through will probably never occur, because it should already be covered by the validation, but we should still play it safe.

Could you add this as a comment here? You can also add /* istanbul ignore next */ to drive home the point that this code should never get hit.

wjhsf · 2024-01-19T21:46:39Z

...babel-plugin-component/src/__tests__/fixtures/wire-decorator/ignores-object-method/actual.js

This diff is git being silly. I deleted the old file because it was a duplicate of another test, and the new file is unrelated.

nolanlawson

LGTM overall. Nicely done!

nolanlawson · 2024-01-19T21:50:13Z

packages/@lwc/babel-plugin-component/src/decorators/wire/transform.ts

+                        const value = t.isNullLiteral(p.key) ? null : p.key.value;
+                        return t.stringLiteral(String(value));
+                    }
+                    // If it's not an identifier or primitive literal then it's a computed expression


Could you add this as a comment here? You can also add /* istanbul ignore next */ to drive home the point that this code should never get hit.

nolanlawson · 2024-01-19T21:58:31Z

packages/@lwc/babel-plugin-component/src/decorators/wire/validate.ts

+
+    for (const prop of config.get('properties')) {
+        // Only validate {[computed]: true} object properties; {static: true} props are all valid
+        // and we ignore {...spreads} and {methods(){}}


I just tested, and you're correct that the existing compiler ignores spreads and methods. This is an oversight in our validation logic.

Could you please open a follow-up GH issue for this? This is the kind of thing that may need to be handled with API versioning, since it may already be "exploited" in the wild (spreads in particular).

nolanlawson · 2024-01-19T21:59:17Z

packages/@lwc/babel-plugin-component/src/decorators/wire/validate.ts

+    for (const prop of config.get('properties')) {
+        // Only validate {[computed]: true} object properties; {static: true} props are all valid
+        // and we ignore {...spreads} and {methods(){}}
+        if (!prop.isObjectProperty() || !prop.node.computed) continue;


It's not actually enforced in our ESLint rules, but I think by convention we always use curly brackets for cases like this.

Suggested change

if (!prop.isObjectProperty() || !prop.node.computed) continue;

if (!prop.isObjectProperty() || !prop.node.computed) {

continue;

}

Ditto for other cases below.

nolanlawson · 2024-01-19T22:04:26Z

packages/@lwc/errors/src/compiler/error-info/lwc-class.ts

+
+    COMPUTED_PROPERTY_MUST_BE_CONSTANT_OR_LITERAL: {
+        code: 1200,
+        message: 'Computed property in @wire config must be a constant or primitive literal.',


Suggested change

message: 'Computed property in @wire config must be a constant or primitive literal.',

message: 'Computed property in @wire config must be a constant or primitive literal, such as a string or Symbol.',

Most developers probably don't know what the words "constant or primitive literal" mean – this gives a bit of a hint. 🙂

nolanlawson · 2024-01-19T22:08:08Z

packages/@lwc/babel-plugin-component/src/decorators/wire/validate.ts

+        validateWireId(id, path, state);
+        if (config) validateWireConfig(config, path, state);
+    } else {
+        // Single argument: should just be id


The docs are not clear, but it does seem that the config is optional. A quick search for @wire in SourceGraph reveals several instances of @wire being used with just the single argument. We should probably add a test for this.

Also you are correct, this line is not hit per code coverage.

nolanlawson · 2024-01-19T22:11:25Z

🚨 Yes, it does introduce a breaking change.

I agree, but to be clear I don't think this merits a major version bump. We only recently allowed computed properties in @wire, and 250 has not shipped to third-party component authors yet.

wjhsf added 5 commits January 16, 2024 14:27

refactor(@wire): split parameter validation into individual methods

dc1fe80

chore(tests): remove duplicate tests

63eb2e5

duplicate of decorator-accepts-a-member-expression

test(@wire): add tests for spreads, methods, and numbers

d509cb6

test(@wire): update test for computed identifiers and primitives

bae05e1

feat: allow computed properties that are constants or primitive literals

813982f

wjhsf changed the title ~~feat: allow computed properties in @wire if they are constants or primitives~~ feat: allow computed properties in @wire if they are constants or primitives @W-14785085 Jan 18, 2024

wjhsf added 6 commits January 18, 2024 17:12

refactor: less nesting

2731f9a

test(@wire): add test for template literal computed prop

81ddb0b

test(@wire): add test for let variable computed prop

5152552

test(@wire): add test for expression in computed prop

1f8255f

test(@wire): add test for regexp literal computed prop

676694d

test(@wire): add test for bigint literal computed prop

0c1e1a3

wjhsf mentioned this pull request Jan 19, 2024

@wire computed properties should be restricted to values that cannot change #3956

Open

chore: add github issues to TODOs

186d426

wjhsf commented Jan 19, 2024

View reviewed changes

wjhsf marked this pull request as ready for review January 19, 2024 21:46

wjhsf requested a review from a team as a code owner January 19, 2024 21:46

nolanlawson approved these changes Jan 19, 2024

View reviewed changes

Merge branch 'master' into wjh/helpful-wire-error

5bed7b5

wjhsf merged commit 8e90e92 into master Jan 22, 2024
9 checks passed

wjhsf deleted the wjh/helpful-wire-error branch January 22, 2024 21:18

wjhsf mentioned this pull request Jan 31, 2025

feat(wire): allow arbitrary computed properties in @wire config @W-17206172 #5183

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow computed properties in @wire if they are constants or primitives @W-14785085 #3955

feat: allow computed properties in @wire if they are constants or primitives @W-14785085 #3955

wjhsf commented Jan 18, 2024 •

edited

Loading

wjhsf commented Jan 19, 2024

wjhsf Jan 19, 2024

nolanlawson Jan 19, 2024 •

edited

Loading

wjhsf Jan 19, 2024

nolanlawson Jan 19, 2024

wjhsf Jan 19, 2024

nolanlawson left a comment

nolanlawson Jan 19, 2024

nolanlawson Jan 19, 2024

nolanlawson Jan 19, 2024

nolanlawson Jan 19, 2024

nolanlawson Jan 19, 2024 •

edited

Loading

nolanlawson commented Jan 19, 2024

	message: 'Computed property in @wire config must be a constant or primitive literal.',
	message: 'Computed property in @wire config must be a constant or primitive literal, such as a string or Symbol.',

feat: allow computed properties in @wire if they are constants or primitives @W-14785085 #3955

feat: allow computed properties in @wire if they are constants or primitives @W-14785085 #3955

Conversation

wjhsf commented Jan 18, 2024 • edited Loading

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

GUS work item

wjhsf commented Jan 19, 2024

Choose a reason for hiding this comment

nolanlawson Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolanlawson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolanlawson Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

nolanlawson commented Jan 19, 2024

wjhsf commented Jan 18, 2024 •

edited

Loading

nolanlawson Jan 19, 2024 •

edited

Loading

nolanlawson Jan 19, 2024 •

edited

Loading