-
Notifications
You must be signed in to change notification settings - Fork 394
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
feat(signals): add a way to set symbol as key #4665
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking a bit more about this, if we want to prevent "fake" signals, we can't attach anything to the signal class, because end users can sniff anything we'd want to check:
// symbol as value
class Signal {
__id = Symbol('private-lwc-signal');
}
new Signal().__id // 💥
class Signal {
// symbol as prop
[Symbol('private-lwc-signal')]: true;
}
Object.getOwnPropertySymbols(Signal.prototype)[0]; // 💥
class Signal {
// symbol not directly accessible
#identity = Symbol('private-lwc-signal');
__isLwcSignal() { /* do something with #identity */ }
}
class FakeSignal extends Signal {
__isLwcSignal() { return true } // 💥
}
Rather than exposing the symbol, we need to expose a checker function:
// @lwc/engine-core
import { setSignalValidator, Signal } from "@lwc/signals"
const trustedSignals = new WeakSet()
const trustThisSignal = (signal: Signal) => trustedSignals.has(signal);
setSignalValidator(trustThisSignal);
// ...
if (trustedSignals.has(someSignal)) { ... }
// @lwc/signal
let trustThisSignal: undefined | (() => void)
const setSignalValidator = once(fn => trustThisSignal = fn)
class Signal {
constructor () {
trustThisSignal?.(this)
}
}
By doing this, we're not altering the Signal
interface, and the setSignalValidator
function is a no-op after we use it, so there's no way for end users to hijack the system.
This is a really good point. Maybe at this point we should be using a import { setSignalValidator } from '@lwc/engine-dom'
import { setSignalValidator as setSignalValidator2 } from '@lwc/signals'
const weakSet = new WeakSet()
setSignalValidator(weakSet) // throws if called twice
setSignalValidator2(weakSet) // throws if called twice Then the constructor for |
Added a shared |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
||
### setSignalValidator() | ||
|
||
This experimental API enables the addition of a signal as a trusted signal. If the [`ENABLE_EXPERIMENTAL_SIGNALS`](https://github.com/salesforce/lwc/blob/master/packages/%40lwc/features/README.md#lwcfeatures) feature is enabled, any signal value change will trigger a re-render. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This experimental API enables the addition of a signal as a trusted signal. If the [`ENABLE_EXPERIMENTAL_SIGNALS`](https://github.com/salesforce/lwc/blob/master/packages/%40lwc/features/README.md#lwcfeatures) feature is enabled, any signal value change will trigger a re-render. | |
This experimental API enables the addition of a signal as a trusted signal. If the [`ENABLE_EXPERIMENTAL_SIGNALS`](https://github.com/salesforce/lwc/blob/master/packages/%40lwc/features/README.md#lwcfeatures) feature is enabled, any signal value change will trigger a re-render. | |
If `setSignalValidator` is called more than once, it will throw an error. If it is never called, then no trusted signal validation will be performed. The same `setSignalValidator` API must be called on both `@lwc/engine-dom` and `@lwc/signals`. |
Details
Continuing this as work that entailed from feedback from #4650
lwc-engine
setSignalIdentity
API, the symbol is used to recognize a signal known to engine at runtimelwc-signals
Any runtime bootstrap service should be able to configure these unique symbols.
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
GUS work item
W-16994654