Skip to content
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

[WNMGDS-2759] Fix nesting web components in other web components and re-renders #3053

Merged
merged 35 commits into from
Apr 23, 2024

Commits on Apr 18, 2024

  1. Exploring different ways of creating separate bundles for individual …

    …web components
    
    This one uses multiple entry points and manually splitting the "base" (shared) code
    pwolfert committed Apr 18, 2024
    Configuration menu
    Copy the full SHA
    ba8d14d View commit details
    Browse the repository at this point in the history
  2. 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.
    pwolfert committed Apr 18, 2024
    Configuration menu
    Copy the full SHA
    9effbde View commit details
    Browse the repository at this point in the history
  3. Configuration menu
    Copy the full SHA
    bfe9643 View commit details
    Browse the repository at this point in the history
  4. 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
    pwolfert committed Apr 18, 2024
    Configuration menu
    Copy the full SHA
    1f367f1 View commit details
    Browse the repository at this point in the history
  5. 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.
    pwolfert committed Apr 18, 2024
    Configuration menu
    Copy the full SHA
    1209ee5 View commit details
    Browse the repository at this point in the history
  6. 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
    pwolfert committed Apr 18, 2024
    Configuration menu
    Copy the full SHA
    ffb6879 View commit details
    Browse the repository at this point in the history
  7. 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
    pwolfert committed Apr 18, 2024
    Configuration menu
    Copy the full SHA
    3a57325 View commit details
    Browse the repository at this point in the history
  8. Using async="false" works, but...

    but if I refresh fast enough, I get the button-inside-a-button problem
    pwolfert committed Apr 18, 2024
    Configuration menu
    Copy the full SHA
    74e024a View commit details
    Browse the repository at this point in the history
  9. 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
    pwolfert committed Apr 18, 2024
    Configuration menu
    Copy the full SHA
    4b2816d View commit details
    Browse the repository at this point in the history
  10. Configuration menu
    Copy the full SHA
    5dac1b0 View commit details
    Browse the repository at this point in the history
  11. Configuration menu
    Copy the full SHA
    724283e View commit details
    Browse the repository at this point in the history
  12. Configuration menu
    Copy the full SHA
    87cc1ec View commit details
    Browse the repository at this point in the history
  13. Configuration menu
    Copy the full SHA
    b0a6315 View commit details
    Browse the repository at this point in the history

Commits on Apr 19, 2024

  1. Configuration menu
    Copy the full SHA
    5f5de99 View commit details
    Browse the repository at this point in the history

Commits on Apr 22, 2024

  1. 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.
    pwolfert committed Apr 22, 2024
    Configuration menu
    Copy the full SHA
    9bfb7cb View commit details
    Browse the repository at this point in the history
  2. 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.
    pwolfert committed Apr 22, 2024
    Configuration menu
    Copy the full SHA
    052e7a5 View commit details
    Browse the repository at this point in the history
  3. 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.
    pwolfert committed Apr 22, 2024
    Configuration menu
    Copy the full SHA
    56bdd0b View commit details
    Browse the repository at this point in the history
  4. Oh wow, I think this works

    Idea came from [this Preact issue thread](preactjs/preact#3444)
    pwolfert committed Apr 22, 2024
    Configuration menu
    Copy the full SHA
    c007ec6 View commit details
    Browse the repository at this point in the history
  5. Configuration menu
    Copy the full SHA
    eca4a10 View commit details
    Browse the repository at this point in the history
  6. Clean up more debug logging

    pwolfert committed Apr 22, 2024
    Configuration menu
    Copy the full SHA
    8a3f476 View commit details
    Browse the repository at this point in the history
  7. Update parse module tests

    pwolfert committed Apr 22, 2024
    Configuration menu
    Copy the full SHA
    24b8be8 View commit details
    Browse the repository at this point in the history
  8. Configuration menu
    Copy the full SHA
    35d917c View commit details
    Browse the repository at this point in the history
  9. Update render doc comment

    pwolfert committed Apr 22, 2024
    Configuration menu
    Copy the full SHA
    3feebe6 View commit details
    Browse the repository at this point in the history
  10. 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
    pwolfert committed Apr 22, 2024
    Configuration menu
    Copy the full SHA
    2438515 View commit details
    Browse the repository at this point in the history

Commits on Apr 23, 2024

  1. Update some comments

    pwolfert committed Apr 23, 2024
    Configuration menu
    Copy the full SHA
    feab276 View commit details
    Browse the repository at this point in the history
  2. Update snapshots

    pwolfert committed Apr 23, 2024
    Configuration menu
    Copy the full SHA
    b0f3b55 View commit details
    Browse the repository at this point in the history
  3. Configuration menu
    Copy the full SHA
    9cf71ed View commit details
    Browse the repository at this point in the history
  4. Configuration menu
    Copy the full SHA
    2429f51 View commit details
    Browse the repository at this point in the history
  5. Configuration menu
    Copy the full SHA
    f089f16 View commit details
    Browse the repository at this point in the history
  6. Configuration menu
    Copy the full SHA
    421a4d6 View commit details
    Browse the repository at this point in the history
  7. Configuration menu
    Copy the full SHA
    7fd36d9 View commit details
    Browse the repository at this point in the history
  8. 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.
    pwolfert committed Apr 23, 2024
    Configuration menu
    Copy the full SHA
    4eeda48 View commit details
    Browse the repository at this point in the history
  9. 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!
    pwolfert committed Apr 23, 2024
    Configuration menu
    Copy the full SHA
    9d10be7 View commit details
    Browse the repository at this point in the history
  10. Configuration menu
    Copy the full SHA
    43ec239 View commit details
    Browse the repository at this point in the history
  11. Configuration menu
    Copy the full SHA
    fd88b24 View commit details
    Browse the repository at this point in the history