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

prerender: rehydrated components (pages) added twice to the DOM #2366

Closed
peterpeterparker opened this issue Apr 13, 2020 · 33 comments
Closed
Labels

Comments

@peterpeterparker
Copy link
Contributor

Stencil version:
"stencil/core": "^1.12.3"

I'm submitting a:
[X] bug report

Current behavior:
When I upgrade my apps to Stencil v1.12.3 and run a prerender build (npm run build --prerender), the components (pages) are added twice to the DOM!

I can confirm that if I run npm run start these are correctly added only once.

Not sure if the problem is linked to Stencil or the Ionic router, which I'm using.

Expected behavior:
Component added only once to the DOM.

Steps to reproduce:

I provide a sample repo.

git clone https://github.com/peterpeterparker/twice-component
npm run build

and then servewww with a local web server.
Screenshot:

Capture d’écran 2020-04-13 à 09 54 57

@ionitron-bot ionitron-bot bot added the triage label Apr 13, 2020
@peterpeterparker peterpeterparker changed the title pretender add component (pages) twice to the DOM prerender add component (pages) twice to the DOM Apr 13, 2020
@peterpeterparker peterpeterparker changed the title prerender add component (pages) twice to the DOM prerender: component (pages) added twice to the DOM Apr 13, 2020
@peterpeterparker peterpeterparker changed the title prerender: component (pages) added twice to the DOM prerender: rehydrated components (pages) added twice to the DOM May 9, 2020
@Paul-Hebert
Copy link

@peterpeterparker , were you able to resolve this in your project? Any info on a workaround? Thanks!

@peterpeterparker
Copy link
Contributor Author

@Paul-Hebert I wasn't. Still unsure if something not correctly configured or "just" an issue in case of re-hydratation when the Ionic router is used.

I actually reported it today and they (Ionic team) might have a look at it.

@Paul-Hebert
Copy link

Thanks @peterpeterparker ! I'm also in the awkward spot of not knowing whether I'm doing something wrong or if there's a bug. Would you mind updating me if you learn more? I'll post back here if I can figure it out. Thanks!

@peterpeterparker
Copy link
Contributor Author

@Paul-Hebert sure

@Paul-Hebert
Copy link

By the way, here's a simplified example that reproduces this issue for me:

My simplified components

import { Component, h } from "@stencil/core";

@Component({
  tag: "c4-button",
  shadow: true,
})
export class C4Button {
  render() {
    return (
      <button>
        <slot />
      </button>
    );
  }
}
import { Component, h} from "@stencil/core";

@Component({
  tag: "c4-page",
})
export class C4Page {
  render() {
    return (
      <div>
        <slot />
      </div>
    );
  }
}

My simplified document I'm hydrating

<!DOCTYPE html>
<html>

<head>
  <script src="/stencil/stencil-components.js"></script>
</head>

<body>
  <c4-page>
      <c4-button>I'm a c4-button!</c4-button>
  </c4-page>
</body>
</html>

My Node/Express code running the hydrate script

app.engine("stencil-ssr", function (filePath, options, callback) {
  fs.readFile(filePath, function (err, content) {
    if (err) return callback(err);

    const htmlString = content.toString();

    return renderToString(htmlString).then((resp) => {
      return callback(null, resp.html);
    });
  });
});

(Though I see the same issue when prerendering instead of doing SSR)

The output

Note how there's a second button with an empty slot. It's invisible in this screenshot, but when I have button CSS applying padding/backgrounds, etc. it becomes visible:

Screen Shot 2020-05-13 at 2 26 57 PM

If I remove shadow DOM from the c4-button, I get this similar but different issue. Now the duplicate buttons are nested:

Screen Shot 2020-05-13 at 2 36 35 PM

If anyone could point me towards a solution or something I'm doing wrong I'd really appreciate it.
I'd love to improve the performance of my Stencil app with preloading but this is a major blocker in that effort.

Thanks! 😁

@Paul-Hebert
Copy link

I just played with this again. In my actual app there are a number of different components with this issue, but there's not a clear pattern of why it happens. Sometimes it happens when it's just a wrapper div. When it happens these things seem to be consistent

  • the elements use Shadow DOM
  • the direct child of the Shadow DOM gets duplicated
  • the duplicate does not get any slot content (if there is a slot)
  • the duplicate does not get the classes Stencil adds during hydration (e.g. sc-c4-container)

Hopefully that extra detail is helpful 🙂

@peterpeterparker
Copy link
Contributor Author

peterpeterparker commented Jun 10, 2020

I did some experiment and found something I documented on slack, just gonna copy it here it that can help.

After build my dummy app with the ion-router and ion-nav, my index.html contains:

<ion-nav s-id=14>
     <!--s.14.0.0.0.-->
     <app-home s-id=15>

This leads to the rehydratation issue.

But if I correct manually the elements rendering information as following:

<ion-nav s-id=14>
     <!-- Remore comment, important, and add c-id attribut -->
     <app-home s-id=15 c-id=14.0.0.0>

Then the problem is solved!

Also, without removing the comment it would actually work too but then thenc-id has to be incremented

<ion-nav s-id=14>
     <!--s.14.0.0.0.-->
     <app-home s-id=15 c-id=14.0.0.1>

@peterpeterparker
Copy link
Contributor Author

@Paul-Hebert can you give a try to the PR @ #2508 I just provided?

It does fix my issue but I would be really happy to hear some second thoughts and tests.
It is my try out to bring the subject forward.

adamdbradley added a commit that referenced this issue Jun 17, 2020
manucorporat pushed a commit that referenced this issue Jun 17, 2020
Co-authored-by: peterpeterparker <david.dalbusco@outlook.com>
Co-authored-by: Adam Bradley <adamdbradley@users.noreply.github.com>
@peterpeterparker
Copy link
Contributor Author

This should now be solved (any feedback appreciated).

The PR #2517 has been merged.

@Paul-Hebert
Copy link

Hey @peterpeterparker sorry I missed your ping on this! I'll test this out tomorrow! Thanks for the PR!

@Paul-Hebert
Copy link

Actually I just remembered we have the day off tomorrow for Juneteenth. I'll try to check this out today or Monday

@peterpeterparker
Copy link
Contributor Author

@Paul-Hebert no worries, let me know if it (hopefully) works out!

@Paul-Hebert
Copy link

Dang, unfortunately I'm still running into duplication errors. Here are the steps I followed to test this out:

  1. Update stencil core package to 1.15.0-4 in package.json
  2. Run npm i
  3. Run stencil build (and see that the hydrate script is rebuilt)
  4. Start the Express app that consumes the stencil hydrate script and the stencil components script
  5. See duplicate slotted elements

I'm currently working on some other tasks so don't have more time to dig into this now, but let me know if there's any more info I could provide to help out or anything else I should test. (I ran this against my simplified test case as well as my more complex app)

Thanks for your work on this!

@Paul-Hebert
Copy link

If I find some time I'll whip up a repro repo, but it might be a bit before I have a chance.

@peterpeterparker
Copy link
Contributor Author

@Paul-Hebert dang :(

yes a repro repo would be nice, I'll be happy to try to reproduce it on my laptop. If you can also post an example of the prerendered HTML file I would be interesting (to have a look on the c-id and s-id attributes).

@peterpeterparker
Copy link
Contributor Author

Ping me

@Paul-Hebert
Copy link

Hey @peterpeterparker , sorry for the delay. Work's been busy but I put together reproduction repo: https://github.com/cloudfour/stencil-ssr-bug-reproduction

This is based off of my main (private) project repo with unnecessary sections stripped out.

Here's the steps I'm going through to reproduce:

  1. Check out repo
  2. run npm i in the root directory as well as /stencil-components and express-app to install dependencies
  3. From the root, run npm run start:stencil to start the stencil server. See a single button render
  4. From the root, run npm run build. This builds my stencil dist folder and my hydrate script. It also copies the dist folder into my Express app so the components script can be used.
  5. From the root, run npm run start:express to start the Express app. Go to http://localhost:3000/ . See that there's a second empty button in addition to the first button.

As I was working through this I realized I had questions around a few things related to the error:

  • Am I loading the script from the dist folder correctly in my Express app? Should I be doing something else when using the hydrate script?
  • Is the baseUrl in my stencil.config.ts file correct? What is that used for?

If you could take a look I'd really appreciate it. Hopefully I'm just doing something wrong in my repo. Or, if there is a Stencil issue hopefully this helps to troubleshoot it. Thanks!

@Paul-Hebert
Copy link

Oh, here are screenshots of what I'm seeing:

Correct
Screen Shot 2020-06-24 at 9 41 44 AM

Incorrect (SSR)
Screen Shot 2020-06-24 at 9 41 57 AM

@peterpeterparker
Copy link
Contributor Author

peterpeterparker commented Jun 24, 2020

@Paul-Hebert cool, thx for the repo and details!

I can reproduce the problem too and do see why too

In the prerendered index.html the button is parser as following

<c4-button class="sc-c4-button-h hydrated" s-id=2 c-id=0.2>

but its parent is s-id=1. if I correct the c-id as following

<c4-button class="sc-c4-button-h hydrated" s-id=2 c-id=1.2>

then prerendering works out. so the question is why 0.2 instead of 1.2.

I've to add some console.log to stencil, let me try

@peterpeterparker
Copy link
Contributor Author

Ok so the PR I implemented kick in and calculate the right c-id but later it is overidden, not sure why yet

[ INFO ]  Hydrate
           DAVID PR, C4-BUTTON, 1.1.0.1

[ INFO ]  Hydrate
           LATER, C4-BUTTON, 0.2

@peterpeterparker
Copy link
Contributor Author

Did not find the solution tonight. A bit busy with clients' projects next days but I'll comeback to it, I can reproduce and see where the heck is happening.

@Paul-Hebert
Copy link

Awesome, thank you @peterpeterparker !

@peterpeterparker
Copy link
Contributor Author

@Paul-Hebert can you open a new issue?

I do have a solution but it breaks all tests, that's why I am a bit confuse. I do see with your test repo that the problem is this indexation and the solution is modifying 0.2 to 1.2 or 1.0.2. Therefore I do not what I am missing or if the indexing in the test can be updated. It needs input from the team, I do not want to submit a PR in which I modify all existing tests and break something.

My solution would be the following. In vdom-annotations modify the hostId of the orgLocationNodes to not always be 0 be rather reflect the s-id of the node, if one was set during parseVNodeAnnotations.

Is:

if (hostId == null) {
          hostId = 0;

My suggestion:

if (hostId == null) {
     hostId = nodeRef.nodeType === NODE_TYPE.ElementNode && 
                   nodeRef.hasAttribute('s-id') 
                  && !isNaN(parseInt(nodeRef.getAttribute('s-id'))) ? 
                         parseInt(nodeRef.getAttribute('s-id')) - 1 : 0;

What do you think, works for you?

@Paul-Hebert
Copy link

Hey @peterpeterparker,

Sorry for the delay. I've been out of the office doing some socially-distant dispersed camping in the woods. I'm focused on client work today but will try out that solution tomorrow and open a new issue.

Thank you for all your help on this!

@peterpeterparker
Copy link
Contributor Author

@Paul-Hebert no worries, busy too these days and next weekend too. Ping me when you've got time, no rush.

@guidoknoll
Copy link

Hello,

i'm running into that issue too. Any news to this problem?

I'd be very thankful for a fix, sadly i can't really provide any help to the suggested solution above.

@Paul-Hebert
Copy link

Hey @peterpeterparker , thanks again for all your help here! Sorry for the super long delay in trying this out.

I think this fixed the issue, but I had to test it in a kind of roundabout way.

I was planning to do the following:

  1. Check out the stencil core repo
  2. Make your change
  3. Build the project
  4. Copy the build output into my bug repro repo, replacing my existing copy of Stencil core.

However, it's not clear to me the correct way to build the stencil core project. I tried build, build.prod and release.prepare. Each of these generated a /build directory with a different structure than I expected. It didn't match the structure of the Stencil Core in my project's Node Modules folder.

I feel like I'm probably missing something obvious there, so if you could provide some guidance on the proper way to build that I can try it out that way.

What I Did Instead

I ended up making this change directly in the package in my Node Modules package. When I made the change here it appears to fix my problem! 🎉

(I had to switch NODE_TYPE.ElementNode to 1 for this to work. Based on inline comments it looks like this change is automatically made during compilation.)

Next Steps

Let me know if there's anything else you'd like me to do to test this.

You also mentioned creating a new issue about this. Is there any specific info I should include there? I was planning to include the following:

  1. A description of the issue
  2. Screenshots
  3. A link to my bug repro repo
  4. A link to your comment describing a fix.

Thanks David!

@peterpeterparker
Copy link
Contributor Author

@Paul-Hebert nice to hear from you and happy to hear that you were able to try it out.

I am not sure anymore if I use the build or build:pro command.

Anyway yes I think we should open a new issue, because this one begins to be quite long and as I said in the workaround, it breaks all tests therefore it definitely needs inputs from the Ionic/Stencil team.

Regarding content of the issue, I think what you describe is what's needed. Please do post the link to the new issue in this one, so I can follow ;)

And thx in advance 👍

@Paul-Hebert
Copy link

Here you go @peterpeterparker: #2611

Thanks again for all your help!

@peterpeterparker
Copy link
Contributor Author

Nice @Paul-Hebert !

@CollinHerber
Copy link

CollinHerber commented Jan 31, 2023

I just upgraded to Stencil 3.0 and I am noticing this issue. On the created link I am not seeing any resolution. Has there been any further development on this?

Edit: One thing to point out is there is only a single container element created.
This is what is observed

<my-component> 
    <div> component markup </div>
    <div> component markup </div>
</my-component>

So based on this it does appear to be during the hydration of the component.

Edit2:
Further investigation suggests that this is only happening for component that have nested stencil components.
So the hydrated markup looks something like this

<my-component> 
    <nested-component>
        <div> component markup </div>
        <div> component markup </div>
    </nested-component>
</my-component>

<nested-component>
      <div> component markup </div>
</nested-component>

@rwaskiewicz
Copy link
Contributor

@CollinHerber can you do me a favor and please create a new issue with a reproduction case for the team and myself?

@CollinHerber
Copy link

We ended up finding out that switching our components to shadow:true fixed the issue and have been rolling with the large amount of changes that creates for us with switching to shadow dow. I hope to have some time in the next week or two to get that to you but we are coming up to release time here and it's very busy. I will tag you when that happens though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants