Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wjhsf opened this issue Jan 19, 2024 · 7 comments
Open

Comments

@wjhsf
Copy link
Contributor

wjhsf commented Jan 19, 2024

To enable reactivity, the @wire decorator transforms code in such a way that computed properties are re-computed whenever updates are needed. This means that variables used as property keys must have string representations that never change.

Example transformation The following snippets are excerpts, not complete code.
// input.js
const symbol = Symbol.for("key");
export default class Test extends LightningElement {
  @wire(getFoo, {
    [symbol]: "$prop1",
    string: "$prop2",
    notReactive: 'just hanging out'
  })
  wiredFoo;
}
// output.js
const symbol = Symbol.for("key");
class Test extends LightningElement {
  wiredFoo;
}
_registerDecorators(Test, {
  wire: {
    wiredFoo: {
      adapter: getFoo,
      dynamic: [symbol, "string"],
      config: function ($cmp) {
        return {
          notReactive: 'just hanging out',
          [symbol]: $cmp.prop1,
          string: $cmp.prop2
        };
      }
    }
  }
});

For reactivity to properly function, the object returned by the config method must always have the keys in the dynamic array.

In #3955, we updated the @wire validation to only permit identifiers that were declared as a const. This prevents accidentally changing the value and breaking reactivity. However, this is not a complete solution, as the string representation of an object may change (e.g. { toString: Math.random }). Additionally, the solution does not permit constants that were imported from another file, even if those constants would otherwise be acceptable to use. (A simple workaround for the import problem is to declare a new constant in the component file.) We should validate that the value for a constant is not an object; only primitives should be allowed as computed property keys.

Additionally, in the same PR we allowed primitives to be used directly as property keys, as that is perfectly valid JavaScript with a static string representation. We did not permit template literals, as they contain expressions that can change (e.g. `${Math.random()}`). We may want to do analysis of template literals, and allow them to be used if we can determine that they have a static string representation (e.g. `without expressions`, `with ${1} static expression`).

For example, the following is currently allowed, but should not be.

// every time the property is computed, it results in a different value
const toStringChanges = { toString: Math.random }
@wire(getFoo, { [toStringChanges]: "$prop" }) nonDeterministicObject;

And following examples are currently not allowed, but should be.

// imported constants should be treated the same as constants declared in the file
import { IMPORTED_CONSTANT } from "./config";
@wire(getFoo, { [IMPORTED_CONSTANT]: "$prop" }) importedConstant;

// the computed string will never change for these template literals
@wire(getFoo, [`no expressions`]: "$prop" }) noExpressions;
@wire(getFoo, [`${1} expression`]: "$prop" }) oneExpression;
@wjhsf wjhsf added the Up for grabs Issues that are relatively small, self-contained, and ready for implementation label Jan 19, 2024
@wjhsf
Copy link
Contributor Author

wjhsf commented Jan 19, 2024

The minimal-change approach to this would be to perform static analysis to try to resolve values and determine whether or not they are primitives. However, a more radical approach might be to re-write how the computed properties are used by taking a snapshot. This would avoid the footgun of changing a variable that is used as a computed key, and it would also allow us to support any expression, not just identifiers and literals.

An example transformation might look like this:

const symbol = Symbol.for("key");
export default class Test extends LightningElement {
  @wire(getFoo, {
    [symbol]: "$prop1",
    [Math.random()]: "$prop2",
    notReactive: 'just hanging out'
  })
  wiredFoo;
}
// output.js
const symbol = Symbol.for("key");
class Test extends LightningElement {
  wiredFoo;
}
const keys = [symbol, Math.random()].map(String)
_registerDecorators(Test, {
  wire: {
    wiredFoo: {
      adapter: getFoo,
      dynamic: [symbol, "string"],
      config: function ($cmp) {
        return {
          notReactive: 'just hanging out',
          [keys[0]]: $cmp.prop1,
          [keys[1]]: $cmp.prop2
        };
      }
    }
  }
});

@nolanlawson
Copy link
Collaborator

Note this is technically a breaking change, but since this is a rarely-used API and we just recently unblocked it for non-strings, it's probably still safe to make this change without API versioning.

@AllanOricil
Copy link
Contributor

I will try this one tomorrow. If I can't figure out where to change I will ask for help.

@AllanOricil
Copy link
Contributor

I went to sleep 7 in the morning last night. Just woke up. So, because I need some rest, I decided to start it next monday.

Copy link

git2gus bot commented Feb 21, 2024

This issue has been linked to a new work item: W-15090483

@AllanOricil
Copy link
Contributor

Ok I have not had time to do it :( sorry
I will let you do it. I can review pr after if u need some help there. Sorry again 😅

@nolanlawson nolanlawson removed the Up for grabs Issues that are relatively small, self-contained, and ready for implementation label Mar 4, 2024
@nolanlawson
Copy link
Collaborator

I think we probably shouldn't call this "up for grabs" since I suspect it would be a breaking change.

We should maybe do it, but we might have to do it behind API versioning or something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants