feat: [WIP] first prototype of node reactions #1431
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ravijayaramappa
changed the title
ravijayaramappa
commented
Jul 29, 2019
| forEach.call(reactionQueue, (entry: ReactionEvent, index: number) => { | ||
| // TODO: Should this be fault tolerant? If one callback failed, should the processing end of continue? | ||
| entry.callback.call(entry.node, entry.type); | ||
| reactionQueue.slice(index, 1); |
There was a problem hiding this comment.
ravijayaramappa
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
caridy
reviewed
Jul 31, 2019
| @@ -19,8 +21,10 @@ export default function queueReactionsForTree( | |||
| reactionTypes: Array<ReactionEventType>, | |||
| reactionQueue: Array<ReactionEvent> | |||
| ): void { | |||
| if (process.env.NODE_ENV !== 'production') { | |||
| assert.invariant(!isUndefined(root), `Expected a dom node but received ${root}`); | |||
There was a problem hiding this comment.
... but received undefined isn't it?
caridy
reviewed
Jul 31, 2019
| if (root instanceof Element && !!root.shadowRoot) { | ||
| const nodeType = nodeTypeGetter.call(root); | ||
| // Perf optimization: Only Element type can have shadowRoot | ||
| if (nodeType === 1 && (root as Element).shadowRoot) { |
There was a problem hiding this comment.
I'm worry about this check here. if the element has a closed shadowRoot, we can't do much. That's why I think my suggestion was to patch attachShadow() so we can always grab the shadowRoot when needed, and we can have a custom mechanism to determine shadow. Something like:
if (!isUndefined(root.$$myInternalFn$$)) {
const sr = root.$$myInternalFn$$();
}There was a problem hiding this comment.
the patch on attachShadow will create $$myInternalFn$$ or a symbol to provide a fn to access the shadow root instance.
There was a problem hiding this comment.
Yup, agreed. Closed shadow was an open question in the RFC too.
About patching attachShadow and adding a field, if I put my security hat on: What stops external users from utilizing the same field to bypass closed shadow?
caridy
reviewed
Jul 31, 2019
| } | ||
|
|
||
| if (isTrue(hasChildNodes.call(root))) { | ||
| // Perf optimization: Element and Document Fragment are the only types that need to be processed | ||
| if ((nodeType === 1 || nodeType === 11) && isTrue(hasChildNodes.call(root))) { |
There was a problem hiding this comment.
probably the call to hasChildNodes then childNodesGetter is slower than just childNodesGetter because you cross the bridge only once.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
caridy
reviewed
Aug 2, 2019
| enumerable: true, | ||
| configurable: true, | ||
| get(this: Node) { | ||
| return isConnected.call(this); |
There was a problem hiding this comment.
this isConnected is the same you import from Node.prototype, I suspect that it must be patched here if it doesn't exist, and to provide shadow semantics.
ekashida
reviewed
Aug 5, 2019
|
@ekashida you're right, this is not different than those other pkgs, we shouild have the same build step. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Benchmark resultsBase commit: |
ravijayaramappa
force-pushed
the
ravi/master/node-reactions
branch
from
24b1326
to
7d0851b
Compare
Benchmark resultsBase commit: |
ravijayaramappa
force-pushed
the
ravi/master/node-reactions
branch
from
7d0851b
to
c0a9db9
Compare
⚠ Performance RegressionBest has detected that there is a Please click here to see more details. Click to view significantly changed benchmarkslwc-engine-benchmark
|
⚠ Performance RegressionBest has detected that there is a Please click here to see more details. Click to view significantly changed benchmarkslwc-engine-benchmark
|
This was referenced Aug 10, 2019
⚠ Performance RegressionBest has detected that there is a Please click here to see more details. Click to view significantly changed benchmarkslwc-engine-benchmark
|
⚠ Performance RegressionBest has detected that there is a Please click here to see more details. Click to view significantly changed benchmarkslwc-engine-benchmark
|
⚠ Performance RegressionBest has detected that there is a Please click here to see more details. Click to view significantly changed benchmarkslwc-engine-benchmark
|
caridy
reviewed
Aug 23, 2019
| } | ||
| if (qualifiedReactionTypes.length > 0) { | ||
| if (ObjectKeys(qualifiedReactionTypes).length > 0) { |
There was a problem hiding this comment.
bitwise flag with be just compare to something different than 0
⚠ Performance RegressionBest has detected that there is a Please click here to see more details. Click to view significantly changed benchmarkslwc-engine-benchmark
|
⚠ Performance RegressionBest has detected that there is a Please click here to see more details. Click to view significantly changed benchmarkslwc-engine-benchmark
|
⚠ Performance RegressionBest has detected that there is a Please click here to see more details. Click to view significantly changed benchmarkslwc-engine-benchmark
|
ravijayaramappa
force-pushed
the
ravi/master/node-reactions
branch
from
035ed11
to
286f00b
Compare
⚠ Performance RegressionBest has detected that there is a Please click here to see more details. Click to view significantly changed benchmarkslwc-engine-benchmark
|
Web Platform Testswpt branch: command: // without node-reactions // with node-reactions |
to preserve native behavior for invalid arguments
⚠ Performance RegressionBest has detected that there is a Please click here to see more details. Click to view significantly changed benchmarkslwc-engine-benchmark
|
⚠ Performance RegressionBest has detected that there is a Please click here to see more details. Click to view significantly changed benchmarkslwc-engine-benchmark
|
⚠ Performance RegressionBest has detected that there is a Please click here to see more details. Click to view significantly changed benchmarkslwc-engine-benchmark
|
for perf reasons
⚠ Performance RegressionBest has detected that there is a Please click here to see more details. Click to view significantly changed benchmarkslwc-engine-benchmark
|
⚠ Performance RegressionBest has detected that there is a Please click here to see more details. Click to view significantly changed benchmarkslwc-engine-benchmark
|
|
Closed in favor of #1504 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
First prototype of node-reactions RFC salesforce/lwc-rfcs#11
Started with #1173 as the base.
Does this PR introduce breaking changes?
No, it does not introduce breaking changes.If yes, please describe the impact and migration path for existing applications.
The PR fulfills these requirements: