[WNMGDS-2759] Fix nesting web components in other web components and re-renders #3053
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
…web components This one uses multiple entry points and manually splitting the "base" (shared) code
It's somewhat "automatic", but one problem is that it's very difficult to determine what's unique to Alert, and we were actually getting better results the manual way.
One bug I'm seeing in the ds-button one is that it renders a button in a button in the Astro example
If you load the `ds-button.js` before `ds-alert.js`, the button in the example alert is fine, but if you reverse them, there's a button in a button.
A bug just came up though where order of loading the files matters for how the components are rendered. If you load the ds-alerts.js before ds-button.js, the button in the alert the Astro example becomes a button inside a button. I think this is because we’re relying on loading the web component scripts at the end in general for the examples, and there might be a better way of doing it
…things but that's not practical. I just wanted to commit this idea before reverting
but if I refresh fast enough, I get the button-inside-a-button problem
In the Astro example, the button renders once and then uses the first render as input (children) for the second render, resulting in a button inside a button. I have an idea for how to avoid this in the future, but I wanted to save this commit as a reference
My dev notes so far: As can be seen in the uncommented comment above, I observed that it the button inside an alert actually turned into a button inside an alert inside a button inside an alert. Now that I write that out, I'm realizing that perhaps when my code above looks for a `.component-root`, it's finding the nested `.component-root` instead of the top-level one. Well, I just logged the results for a query for multiple, and it didn't find multiple. Hmm, I think it's important to only look for a `.component-root` at the root, because it's possible that a child already has a component root but the parent doesn't. I've updated it to only look for `.component-root` in the root, but I'm still getting some double-rendering. My theory is that it's not truly removing the non-component-root elements at the end of the render function, but I'm having trouble proving it. That's the next thing to investigate on Monday. It's Monday. I've outputted the `childNodes` after the removal code, and in the top level Alert, it is indeed failing to remove those elements. Inside the loop, it outputs `Removing " "` three times, but the `childNodes` list before and after the removal code does not change. Hmm, actually the length changes from 6 to 3 even though when the members are printed to the log, it only shows three in both cases. Three of them are those text `" "` nodes. If I print out all items as I loop through, it only prints those three and not six. I wonder if `for (const childNode of this.childNodes)` doesn't actually work. Nope, it doesn't actually iterate through all the true members. Fixed it, and now the double-rendering is gone! There's still an issue with button rendering a second time and nesting itself, but I think I found the root cause of that too, which is that the `dom` I pass to the virtual DOM converter function is the old `.component-root` itself and not a fresh "document" created with its `innerHTML`, which means it includes the class name in the root element in the virtual DOM, which is used for `__children`. Doh! That's not enough, because when we take the rendered `.component-root`, we've actually lost the original input. The whole idea of only keeping `.component-root` doesn't work if we lose the custom element instance in memory. We might also need to keep something like `.component-input` and just hide it.
I don't actually think we need to check `!this.hasAttribute('server')` in our render function, because we should only be calling it from places where we already know the DOM exists. I think we can simplify this code. I'm also about to prototype a new algorthm after talking to Sarah which doesn't include keeping a reference to `__children` and is much simpler.
Right now when the button renders the second time (during the alert's render sequence), the innerHTML printed at the start of the function has an empty template string. That seems to be because the vdom version of the alert's children has an empty template string. I'm trying to solve that issue right now.
Idea came from [this Preact issue thread](preactjs/preact#3444)
…the slots object
I'm afraid some components could potentially trigger that observer unintentionally from within the Preact component. I'll have to do some more testing
pwolfert
added
Type: Fixed
pwolfert
commented
Apr 23, 2024
Comment on lines
-91
to
-92
| __slots = {}; | ||
| __children = void 0; |
There was a problem hiding this comment.
These are now calculated in the render function and don't need to be statically calculated at the beginning and saved.
|
It turns out that 421a4d6 was actually needed for the unit tests but not the browser. I'm working on seeing if I can fix the unit tests rather than include unnecessary DOM-manipulation code in the production. Update: Just going to leave it for now. Future improvement. |
This appears to only be necessary for the unit tests, but right now I'm just going to leave it in, even though it's less performant. I'd like to solve this, but later. I tried switching to the `preact` testing library, but that didn't help.
…elements
This can be tested manually with the following:
```
<ds-alert
variation="success"
heading="You've loaded the web-components example"
class-name="ds-u-margin-y--2"
id="alert"
>
<p id="alert-content">
This is an example of a success alert. If you want to see an error alert, click the
button below.
</p>
<ds-button variation="solid" is-alternate="true" id="the-button" class="ds-u-margin-top--1">Break things</ds-button>
<ds-choice type="checkbox" label="I agree to the terms and conditions" name="agree">
<div slot="checked-children">
<div class="ds-c-alert ds-u-margin-top--1">Cool, we hoped you'd check this box.</div>
</div>
</ds-choice>
</ds-alert>
```
In this case, the alert was stealing the `checked-children` slot from its nested `ds-choice` element, which meant the choice checked children was broken!
zarahzachz
approved these changes
Apr 23, 2024
pwolfert
added a commit
that referenced
this pull request
May 10, 2024
…re-renders (#3053) * Exploring different ways of creating separate bundles for individual web components This one uses multiple entry points and manually splitting the "base" (shared) code * Automatic splitting with vendors (not as good) It's somewhat "automatic", but one problem is that it's very difficult to determine what's unique to Alert, and we were actually getting better results the manual way. * Hey, this a-la-carte method seems to work! * All of the separate bundles One bug I'm seeing in the ds-button one is that it renders a button in a button in the Astro example * Hmm, the order in which you load them seems to make a difference If you load the `ds-button.js` before `ds-alert.js`, the button in the example alert is fine, but if you reverse them, there's a button in a button. * Troubleshooting ordering issues in examples A bug just came up though where order of loading the files matters for how the components are rendered. If you load the ds-alerts.js before ds-button.js, the button in the alert the Astro example becomes a button inside a button. I think this is because we’re relying on loading the web component scripts at the end in general for the examples, and there might be a better way of doing it * Figuring out how this works. Deferring render using a template fixes things but that's not practical. I just wanted to commit this idea before reverting * Using `async="false"` works, but... but if I refresh fast enough, I get the button-inside-a-button problem * On this commit, you can see the button in the alert double render In the Astro example, the button renders once and then uses the first render as input (children) for the second render, resulting in a button inside a button. I have an idea for how to avoid this in the future, but I wanted to save this commit as a reference * Port unit tests for parseHtml from preactement and swap enzyme for testing-library * Port the `define` unit tests from preactement and update * Refactor `convertToVDom` so it is pure and doesn't mutate a custom element object * Shouldn't actually need to take into account previously parsed slots * WIP: Saving this partial solution that doesn't really work * Fixed the double-render, but the `.component-root` has another issue. My dev notes so far: As can be seen in the uncommented comment above, I observed that it the button inside an alert actually turned into a button inside an alert inside a button inside an alert. Now that I write that out, I'm realizing that perhaps when my code above looks for a `.component-root`, it's finding the nested `.component-root` instead of the top-level one. Well, I just logged the results for a query for multiple, and it didn't find multiple. Hmm, I think it's important to only look for a `.component-root` at the root, because it's possible that a child already has a component root but the parent doesn't. I've updated it to only look for `.component-root` in the root, but I'm still getting some double-rendering. My theory is that it's not truly removing the non-component-root elements at the end of the render function, but I'm having trouble proving it. That's the next thing to investigate on Monday. It's Monday. I've outputted the `childNodes` after the removal code, and in the top level Alert, it is indeed failing to remove those elements. Inside the loop, it outputs `Removing " "` three times, but the `childNodes` list before and after the removal code does not change. Hmm, actually the length changes from 6 to 3 even though when the members are printed to the log, it only shows three in both cases. Three of them are those text `" "` nodes. If I print out all items as I loop through, it only prints those three and not six. I wonder if `for (const childNode of this.childNodes)` doesn't actually work. Nope, it doesn't actually iterate through all the true members. Fixed it, and now the double-rendering is gone! There's still an issue with button rendering a second time and nesting itself, but I think I found the root cause of that too, which is that the `dom` I pass to the virtual DOM converter function is the old `.component-root` itself and not a fresh "document" created with its `innerHTML`, which means it includes the class name in the root element in the virtual DOM, which is used for `__children`. Doh! That's not enough, because when we take the rendered `.component-root`, we've actually lost the original input. The whole idea of only keeping `.component-root` doesn't work if we lose the custom element instance in memory. We might also need to keep something like `.component-input` and just hide it. * WIP: I don't think we call our render function on the server I don't actually think we need to check `!this.hasAttribute('server')` in our render function, because we should only be calling it from places where we already know the DOM exists. I think we can simplify this code. I'm also about to prototype a new algorthm after talking to Sarah which doesn't include keeping a reference to `__children` and is much simpler. * New implementation that still has some issues Right now when the button renders the second time (during the alert's render sequence), the innerHTML printed at the start of the function has an empty template string. That seems to be because the vdom version of the alert's children has an empty template string. I'm trying to solve that issue right now. * Oh wow, I think this works Idea came from [this Preact issue thread](preactjs/preact#3444) * Yeah, it works. Let's do some cleanup * Clean up more debug logging * Update parse module tests * The `define` module unit tests caught a bug where I forgot to spread the slots object * Update render doc comment * Added a mutation observer, which works, but.. I'm afraid some components could potentially trigger that observer unintentionally from within the Preact component. I'll have to do some more testing * Update some comments * Update snapshots * Undo alert story decorator removal * Undo all the build changes to save for later pull request * Add some missing doc comments * We don't actually need this code * Can't figure out an alternative to this This appears to only be necessary for the unit tests, but right now I'm just going to leave it in, even though it's less performant. I'd like to solve this, but later. I tried switching to the `preact` testing library, but that didn't help. * Make sure parser doesn't let templates steal slots from child custom elements This can be tested manually with the following: ``` <ds-alert variation="success" heading="You've loaded the web-components example" class-name="ds-u-margin-y--2" id="alert" > <p id="alert-content"> This is an example of a success alert. If you want to see an error alert, click the button below. </p> <ds-button variation="solid" is-alternate="true" id="the-button" class="ds-u-margin-top--1">Break things</ds-button> <ds-choice type="checkbox" label="I agree to the terms and conditions" name="agree"> <div slot="checked-children"> <div class="ds-c-alert ds-u-margin-top--1">Cool, we hoped you'd check this box.</div> </div> </ds-choice> </ds-alert> ``` In this case, the alert was stealing the `checked-children` slot from its nested `ds-choice` element, which meant the choice checked children was broken! * Update choice examples now that nested components work * And update VRT refs for those changed stories
7 tasks
This pull request was closed.
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.
Summary
preactement, modifying them to fit our new way of doing things.The new rendering method adds some complexity, but it also allows us to remove some complexity elsewhere in the custom element code. Special thanks to @zarahzachz for her idea to use a
<template>component instead of a<div>!How to test
Checklist
[WNMGDS-####] Titleor [NO-TICKET] if this is unticketed work.Type(only one) label for this PR, if it is a breaking change, label should only beType: BreakingImpacts, multiple can be selected.If this is a change to design:
If this is a change to code:
yarn test:unit:update) and browser-test snapshots (yarn test:browser:update)