[WNMGDS-2744] Add bundle generation for individual web components #3070
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
thanks to the recent fixes for nesting components
pwolfert
added
Type: Breaking
pwolfert
changed the title
zarahzachz
reviewed
May 3, 2024
zarahzachz
approved these changes
May 3, 2024
There was a problem hiding this comment.
OK so testing in different browsers has different outcomes. In Firefox, Spinner doesn't appear, but these changes seem to have fixed whatever my issue with Chrome was and all components (including Spinner) work/appear fine. Safari also works fine. I'm going to approve because this is probably a "Sarah's weird laptop" issue.
4 tasks
pwolfert
added
Type: Added
pwolfert
added a commit
that referenced
this pull request
May 10, 2024
) * 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 * Fix a merge issue * The examples _just work_ with split bundles now thanks to the recent fixes for nesting components * Don't need these now * Set this back to the way it was * Don't need this file since we have to explicitly call out all its imports anyway in the webpack config * This method of configuring webpack isn't effective * Clean up the config See not about not bothering to support child-design-system config and i18n overrides right now * Remove commented code * TEST: Show that we can import web components from child design systems * Revert "TEST: Show that we can import web components from child design systems" This reverts commit c61b64a. * Revert yarn lock * Update documentation on doc site and CDN indices * TEST: Better test of child design system web components * Revert "TEST: Better test of child design system web components" This reverts commit 1544173. * Don't reference folders before they exist (fixes CI builds) * Add the new bundles to the list of side-effects for the packages * Add month picker import to astro example
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
WNMGDS-2744
See the updated documentation and the comments for details, but there are now two ways of consuming the web components from the CDN: All of them at once or a la carte.
all.jsinstead ofweb-components.js. I'm open to feedback on this. I wanted to differentiate frombase.js, but it'd be nice to not cause a breaking change too...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.