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
refactor(engine-core): replaces vm.context wire connecting/disconnecting #2130
Conversation
⚠ Performance RegressionBest has detected that there is a Please click here to see more details. Click to view significantly changed benchmarks@lwc/engine-dom
@lwc/engine-server
|
const connector = createConnector(vm, fieldNameOrMethod, wireDef); | ||
|
||
wiredConnectors.push(connector); |
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.
const connector = createConnector(vm, fieldNameOrMethod, wireDef); | |
wiredConnectors.push(connector); | |
wiredConnectors.push(createConnector(vm, fieldNameOrMethod, wireDef)); |
|
||
// computeConfigAndUpdate already has boundary protection | ||
if (hasDynamicParams) { | ||
Promise.resolve().then(computeConfigAndUpdate); |
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.
we should probably have this as a utility method somewhere.
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.
at first glance it looks cool, but I want to dig deeper, give me more time.
computeConfigAndUpdate: () => void; | ||
resetConfigWatcher: () => void; | ||
} { | ||
function createConnector(vm: VM, name: string, wireDef: WireDef): WireConnector { |
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.
The amount of closure passing makes the code hard to read. I would convert the connector, config watcher, and context watcher into 3 different classes and pass the object reference around. This would certainly make the code more readable and reduce the overall memory usage due to the closure creation.
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.
🎉 Looking much better. Overall LGTM 👍
private setNewContext = (newContext: ContextValue) => { | ||
// eslint-disable-next-line lwc-internal/no-invalid-todo | ||
// TODO: dev-mode validation of config based on the adapter.contextSchema | ||
if (this.context !== newContext) { |
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
in an arrow function set into a private field seems like the wrong thing to do.
} | ||
|
||
computeConfig() { | ||
return this.configCallback(this.component); |
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 should have boundary protection.
Details
This PR attempts to simplify the logic of how the wiring is done by encapsulating the connect/disconnect logic into the
WireConnector
and all context related logic into theContextWatcher
abstractions.Does this PR introduce breaking changes?
No, it does not introduce breaking changes.