fix(engine): shadow root childNodes #374
Merged
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
caridy
reviewed
Jun 4, 2018
| childNodes: { value : [] }, | ||
| childNodes: { | ||
| get(this: ShadowRoot): Element[] { | ||
| if (process.env.NODE_ENV !== 'production') { |
There was a problem hiding this comment.
this warning is tricky because it only appears when in fallback mode. On the other hands, blacklist will always throw. Maybe we need a grayList that wraps the original functionality with a warning, but still allow you to use it.
caridy
reviewed
Jun 4, 2018
| if (process.env.NODE_ENV !== 'production') { | ||
| assert.logWarning(`childNodes on ${this} returns a live nodelist which is not stable. Use querySelectorAll instead.`); | ||
| } | ||
| return EmptyArray; |
There was a problem hiding this comment.
this is supposed to return empty array only when the element tagName is a slot, otherwise it should return the proper values but as a static list, not a live one. I think that's better for selenium/webdriver, while keeping users out of trouble.
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
caridy
reviewed
Jun 5, 2018
| if (process.env.NODE_ENV !== 'production') { | ||
| assert.logWarning(`childNodes on ${this} returns a live nodelist which is not stable. Use querySelectorAll instead.`); | ||
| } | ||
| const vm = getElementOwnerVM(this) as VM; |
Benchmark resultsBase commit: lwc-engine-benchmark
|
trevor-bliss
reviewed
Jun 5, 2018
| const elm = createElement('x-child-node-parent', { is: Parent }); | ||
| document.body.appendChild(elm); | ||
| const slot = elm.shadowRoot.querySelector('x-child-node-with-slot').shadowRoot.querySelector('slot'); | ||
| expect(slot.childNodes.length).toBe(0); |
There was a problem hiding this comment.
expect(slot.childNodes).toHaveLength(0); will give a better failure message.
Same comment for cases below.
| browser.waitUntil(() => { | ||
| return element.getText() === 'Title:task 1 Completed:false'; | ||
| }, 500, 'expect text to be different after 0.5s'); | ||
| browser.pause(50); |
There was a problem hiding this comment.
Can we use the browser.execute call inside the waitUntil instead of this pause?
There was a problem hiding this comment.
yes! I didn't realize you could do that.
| browser.waitUntil(() => { | ||
| return element.getText() === 'Title:task 1 Completed:false'; | ||
| }, 500, 'expect text to be different after 0.5s'); | ||
| browser.pause(50); |
There was a problem hiding this comment.
Same comment as above - can we add browser.execute to the wait?
Benchmark resultsBase commit: lwc-engine-benchmark
|
trevor-bliss
approved these changes
Jun 6, 2018
caridy
approved these changes
Jun 6, 2018
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
Adding
childNodesshim to shadow root and templated elements.Does this PR introduce a breaking change?