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

dist-hydrate-script output target doesn't respect hydratedFlag config #3606

Closed
nick-vincent opened this issue Sep 14, 2022 · 10 comments · Fixed by #5741
Closed

dist-hydrate-script output target doesn't respect hydratedFlag config #3606

nick-vincent opened this issue Sep 14, 2022 · 10 comments · Fixed by #5741
Assignees
Labels
Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through. ssr-related

Comments

@nick-vincent
Copy link

nick-vincent commented Sep 14, 2022

Stencil version:

@stencil/core@2.18.0

Current behavior:

When using the dist-hydrate-script output target for SSR:

renderToString() always adds the class hydrated instead of respecting the hydratedFlag attribute configuration:

image

Expected behavior:

I would expect hydration via renderToString() to correctly respect the hydratedFlag attribute configuration:

image

GitHub Reproduction Link:

Here is my basic stencil repo: https://github.com/nick-vincent/gradient

I have the hydratedFlag set to attribute here and my output target as dist-hydrate-script here.

When you run npm run start inside web-components, Storybook correctly respects the hydratedFlag config.

However, if you inspect the built files, in /web-components/dist/hydrate/index.js the hydrated class is always added:

addHydratedFlag = e => e.classList.add('hydrated'),

Other Information

Edit: Upon further inspection, it appears that you can configure the name of the hydratedFlag:

hydratedFlag: {
  selector: 'attribute',
  name: 'yo-dawg'
}

And the built file /web-components/dist/hydrate/index.js does change the flag name, but it still uses a class instead of an attribute:

addHydratedFlag = e => e.classList.add('yo-dawg')
@ionitron-bot ionitron-bot bot added the triage label Sep 14, 2022
@alicewriteswrongs
Copy link
Member

@nick-vincent thanks for filing the issue, and for providing a reproduction! It's a big help for us when going through and triaging issues.

I've taken a look at your repro and read through some of the relevant code and I believe that you're right that renderToString does not currently respect the hydratedFlag setting.

Can you explain what you mean by the following?

Upon further inspection, it appears that you can change the classname, but it won't let you use selector: attribute:

I'm just wondering where you changed it such that it affected the built output, and what specifically wasn't working with selector: attribute.

Thanks!

@alicewriteswrongs alicewriteswrongs added the Awaiting Reply This PR or Issue needs a reply from the original reporter. label Sep 19, 2022
@ionitron-bot ionitron-bot bot removed the triage label Sep 19, 2022
@nick-vincent
Copy link
Author

Hi @alicewriteswrongs, thanks for your reply!

Apologies I wasn't clearer about the last bit. What I mean is that you can configure the name of the hydratedFlag:

hydratedFlag: {
  selector: 'attribute',
  name: 'yo-dawg'
}

And the built file /dist/hydrate/index.js does change the flag name, but it still uses a class instead of an attribute:

addHydratedFlag = e => e.classList.add('yo-dawg')

I'd be happy to submit a PR for this, but I had trouble tracking down where this logic lives in the compiler.

@ionitron-bot ionitron-bot bot removed the Awaiting Reply This PR or Issue needs a reply from the original reporter. label Sep 19, 2022
@ionitron-bot ionitron-bot bot added the ionitron: stale issue This issue has not seen any activity for a long period of time label Oct 19, 2022
@ionitron-bot
Copy link

ionitron-bot bot commented Oct 19, 2022

Thanks for the issue! This issue is being closed due to inactivity. If this is still an issue with the latest version of Stencil, please create a new issue and ensure the template is fully filled out.

Thank you for using Stencil!

@ionitron-bot ionitron-bot bot closed this as completed Oct 19, 2022
@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Oct 19, 2022
@rwaskiewicz rwaskiewicz removed the ionitron: stale issue This issue has not seen any activity for a long period of time label Oct 20, 2022
@ionic-team ionic-team unlocked this conversation Oct 20, 2022
@rwaskiewicz rwaskiewicz reopened this Oct 20, 2022
@rwaskiewicz
Copy link
Member

Sorry about that! Reopening

@alicewriteswrongs
Copy link
Member

@nick-vincent sorry to miss your earlier message! I'm going to label this so it can be prioritized. Thanks for reporting!

@alicewriteswrongs alicewriteswrongs added the Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through. label Oct 25, 2022
@ionitron-bot ionitron-bot bot removed the triage label Oct 25, 2022
@KevinCarnaille2
Copy link

Despite dist-hydrate-script itself, it's actually a useful feature to set hydrated with attribute instead of class, because it's very common to use custom classes on our tags when implementing them.

I'm giving an example of why this is critical :)
For instance, with Vue3 if you apply conditional classes (binded from a reactive property) on your webcomponents, it breaks the style because hydrated class is overriden by Vue. If you have the invisiblePrehydration flag set to true, well then you don't see your elements
(In a dist output/cdn/lazy-loaded components context)

<template>
   <my-custom-element :class="{ customClass: isCustomClass }"></my-custom-element>
<template>

<script setup>
import {ref} from 'vue';

const isCustomClass = ref(true); // playing with this overrides hydrated class

</script>

Applying attribute is then a pretty handy solution, I use it for a year now. But with the issue mentioned by @nick-vincent , that I'm facing as well, dist-hydrate-script is not easy to use IMHO.

@krivaten
Copy link

@alicewriteswrongs I'm experiencing the same behavior with dist-custom-elements. Would you prefer a separate issue to be reported, or can it be tacked on to this one?

@alicewriteswrongs
Copy link
Member

Maybe let's go with another issue so we can track it separately in case there's a different root cause for the same behavior? I don't know that's the case, but it will help us to have a dist-custom-elements specific reproduction as well.

If they have the same root cause we'll just close both of them when we fix anyhow!

@FrancoisDF
Copy link

Is there any update on this issue? I'm getting similar problem with hydrated class and sveltekit double hydration (once from stenciljs, then svelte hydration removing the hydrate class). For now we are using

hydratedFlag: {
    name: '',
  },

It's working as expected, but a bit worried about all potential impact of that trick. We also tried hydratedFlag: null but it's ignored similar to selector: 'attribute'.

@alicewriteswrongs
Copy link
Member

Sorry for the slow progress on this issue!

I've just taken another look at it, and I've got a bit of a fix together -- I'm not sure it's the best way of addressing the issue, but I'd love to know if it does fix the problem for anyone who's experiencing this.

You can install it like so:

npm install @stencil/core@4.18.0-dev.1715200204.567f0d2

alicewriteswrongs added a commit that referenced this issue May 15, 2024
This changes how BUILD conditionals get set for the Hydrate script to
ensure that configuration set by users using `hydratedFlag` is
respected.

fixes #3606
STENCIL-609
github-merge-queue bot pushed a commit that referenced this issue May 15, 2024
…5741)

This changes how BUILD conditionals get set for the Hydrate script to
ensure that configuration set by users using `hydratedFlag` is
respected.

fixes #3606
STENCIL-609
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through. ssr-related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants