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

Hydrated Nested Svelte Components Losing Styles #37

Closed
kevmodrome opened this issue Sep 19, 2020 · 15 comments · Fixed by #42
Closed

Hydrated Nested Svelte Components Losing Styles #37

kevmodrome opened this issue Sep 19, 2020 · 15 comments · Fixed by #42

Comments

@kevmodrome
Copy link

kevmodrome commented Sep 19, 2020

I ran across a problem where a hydrated components nested component lost some or all of it's styles. The following component has hydrate-client on it:

<!-- Navigation.svelte --!>
<script>
  import MobileMenu from './MobileMenu.svelte';
  let y;
  $: scrolled = y > 100;
  export let menu;
  let menuOpen = false;
</script>

<style>
  .navcontainer {
    position: fixed;
    top: 0;
    left: 0;
    padding: 25px 0 20px 0;
    width: 100%;
    align-items: center;
    justify-content: center;
    background-color: rgba(0, 0, 0, 0.1);
    transition: background-color 0.3s ease;
    --small: var(--media-lte-sm) none;
    display: var(--small, flex);
  }
  .navcontainer.scrolled {
    background-color: #162b2e;
  }

  a {
    color: #2a8290;
    font-size: 19px;
    margin: 0 24px;
    padding: 4px 20px;
    text-decoration: none;
    display: inline-flex;

    transition: background-color 0.3s ease;
  }

  a:visited {
    color: #2a8290;
  }

  a:hover,
  a:focus {
    background: rgba(255, 255, 255, 0.2);
  }

  a.scrolled {
    color: white;
  }

  a.scrolled:visited {
    color: white;
  }

  .hamburger {
    position: absolute;
    padding: 30px;
    top: 6px;
    right: 6px;

    --small: var(--media-lte-sm) initial;
    display: var(--small, none);
  }
  img {
    height: 16px;
    place-self: center;
  }
  span {
    display: grid;
    grid-gap: 8px;
    grid-template-columns: auto auto;
    justify-items: center;
  }
</style>

<svelte:window bind:scrollY={y} />
<div class="navcontainer" class:scrolled>
  <nav>
    {#each menu as { url, name }}<a href={url} class:scrolled> {name} </a>{/each}

    <a rel="noreferrer" href="https://forms.gle/6PBKXng9jfrvxjhX8" target="_blank" class:scrolled> Sign Up </a>

    <a rel="noreferrer" target="_blank" href="https://twitter.com/sveltesociety" class:scrolled>
      <span> <img src="/dist/static/images/twitter.svg" alt="" /> Twitter </span>
    </a>
  </nav>
</div>
<div class="hamburger" on:click={() => (menuOpen = true)}><img src="/dist/static/images/burger.svg" alt="" /></div>

{#if menuOpen}
  <MobileMenu {menu} on:click={() => (menuOpen = false)} />
{/if}

and

<!-- MobileMenu.svelte --!>
<script>
  export let menu;
</script>

<style>
  .container {
    background: #17353a;
    position: fixed;
    top: 0;
    left: 0;
    right: 0;
    bottom: 0;
    overflow: scroll;
  }
  ul {
    padding-top: 100px;
    padding-bottom: 100px;
    display: grid;
    grid-gap: 40px;
    list-style: none;
  }
  li {
    text-transform: uppercase;
    font-family: Anton;
    font-size: 48px;
    line-height: 120%;
  }
  a:hover {
    color: white;
    opacity: 1;
  }
  a {
    color: var(--sky-blue);
    opacity: 0.6;
    text-decoration: none;
    letter-spacing: 0.6px;
  }
  img {
    padding: 30px;
    position: absolute;
    top: 6px;
    right: 6px;
  }
</style>

<div class="container">
  <ul>
    {#each menu as { name, url }}
      <li><a on:click href="/{url}">{name}</a></li>
    {/each}
    <li><a href="https://forms.gle/6PBKXng9jfrvxjhX8" rel="noreferrer" target="_blank"> Sign up </a></li>
    <li><a target="_blank" href="https://twitter.com/sveltesociety">Twitter</a></li>
  </ul>
</div>
<button on:click> <img src="dist/static/images/close.svg" alt="" /> </button>

The workaround I found is to move the content of MobileMenu.svelte into Navigation.svelte.

@x4080
Copy link

x4080 commented Sep 19, 2020

Yes, I found that also - first time, I was confused, my solution was to move the css inline to global css file - not great solution though.

Maybe because when server rendered some component not appear yet, so css is purged in the client

@nickreese
Copy link
Contributor

nickreese commented Sep 20, 2020

@halafi and I will be looking at this tomorrow.

Relevant file: https://github.com/Elderjs/elderjs/blob/master/src/utils/svelteComponent.ts

Angles to explore:

  1. Could the memory reference to “page” object not be getting persisted during recursion so it never gets added to the stack?
  2. Is the svelte render function not emitting the css?

@kevmodrome
Copy link
Author

From memory it should not be the second option. Rendering a "top-level" component should take into consideration all the sub-components and render it all out with HTML, HEAD and CSS, as long as there isn't any async/await involved.

@halafi
Copy link
Contributor

halafi commented Sep 21, 2020

So I think in this case the SSR component render behaves as expected, no html / css for the mobile menu is there if menuOpen = false during server rendering.

No css emitted for html css.

The client side svelte component mounts the html on 🍔 button click, but doesn't load any css - I guess we should have all styles in additional .css file, but probably emitted by the client side component render in svelteComponent?

--

I didn't find a fix, but a quick workaround:

always render the mobile menu in Navigation component <MobileMenu bind:menuOpen={menuOpen} ... /> and keep the {#if menuOpen}...{/if} inside MobileMenu wrapping the component code, then the styles get emitted and included in the SSR markup

@nickreese
Copy link
Contributor

nickreese commented Sep 21, 2020

@kevmodrome @x4080 @arggh

It appears that the css isn't emitted when there is a boolean that is false-y during SSR. Another work around for V1 is to use process.env.componentType === 'server' to generate a SSR only true boolean. This would mean let menuOpen = false; would become let menuOpen = process.env.componentType === 'server';.

I think it may be worth opening an issue with Svelte on because it appears that NOT emitting CSS on the client is the desired behavior... except when the compiler is given 'ssr'.

Having discussed the options with @halafi, Elder.js has few ways to solve this problem directly due to our complex rollup process.

  1. Use something like ./src/**/*.svelte to glob for all svelte files and have the svelte compiler emit css so it can be managed by rollup plugins and merged into an all.css. The big downside here is that on projects with more than a handful of components re-bundling will take a lot longer during development.
  2. Have all components emit their css into a folder within the ___ELDER___ folder. After the rollup process is done, we then glob that folder, merge the css together into an all.css then copy it where it needs to go.

I like the 2nd option the best, but not sure how to get a script to fire EVERY time rollup runs if it isn't a rollup config.

@arggh
Copy link

arggh commented Oct 10, 2020

I once again encountered this bug when trying to use a modal/dialog component that mounts itself as a new "app". All the styles are lost.

Examples usage of said modal/dialog.

<script>
  import { Confirm } from 'dialog';

  async function handleDelete() {
     const proceed = await Confirm('Really?');
     ....
  }
</script>

Confirm() just mounts the dialog component with specific props as a new app to document.body.

@x4080
Copy link

x4080 commented Oct 31, 2020

@nickreese Is it fixed already in newest release ?

@nickreese
Copy link
Contributor

@x4080 yup. Should be good to go. @arggh should also be fixed. Let me know if you guys hit snags.

@x4080
Copy link

x4080 commented Oct 31, 2020

cool thanks

@armchair-traveller
Copy link

armchair-traveller commented Nov 7, 2020

@nickreese I'm still getting this issue as of v1.1.8 and was able to reproduce it with the template on updated packages (as well as default template with no updated packages) by isolating / stripping components. Should I copy+paste it or is it enough that you're aware it's still an issue?

There's a caveat: This only happens for npm start and the final built version, but not the dev env with 2 terminals (server+rollup).

Additionally, on the new update I'm having a weird issue in my project where changing files during development won't apply after rollup detects the change, and most styles for the entire app disappear forcing me to restart rollup to get it to apply the changes (back to normal). I think these changes not applying doesn't just apply to styles, though, and I'm not confident I can reproduce it on the template... however my codebase doesn't deviate too far from the template besides updated packages.

Just an FYI, if you update all the packages in the template you'll need to install postcss manually as it's not included in the package.json and probably one of the packages stopped including it.

(Very tiny nitpick from before but I noticed the CSS sourcemaps are sort of working now! Which is great except most of the components I'm seeing aren't mapping 1:1 and while the file is all correct, the lines all map to end up at the last line of the style tag. Not sure if you've noticed this yet?)

EDIT: Just wanted to mention the specific issue I'm having is similar to the example above where Kev is toggling a menu. Because of that the let menuOpen = process.env.componentType === 'server' works perfectly for me and it isn't a huge issue compared to having to restart rollup every change.

@nickreese
Copy link
Contributor

@armchair-traveller sorry about the issues. Been digging into css edge cases all week. If you can create a repo that I can clone that would be helpful. I’m surprised falsey values would be excluded with this implementation.

Honestly it appears we’re up against a rock and a hard place with the Svelte ssr component only emitting truth-y styles and with the Svelte plugin changing when SvelteKit is released we either need to write our own Rollup plugin or just write css to a single central file though that doesn’t seem ideal.

Further the Svelte plugin is looking at dropping sourcemap support so that will cause our support to drop as well.

My current thought is to go back to the old CSS implementation without sourcemaps that has falsey value issues and allow users to add their own rollup plugins to handle css files if desired and toggle off adding CSS into the html.

Lots of digging left to do and unfortunately there just isnt an easy one size fits all solution that I see.

@armchair-traveller
Copy link

@nickreese no worries. I'm confident you'll get it resolved in a satisfactory manner.

Repo: https://github.com/armchair-traveller/elderjs-issue-37
As mentioned before, it's the template + components I isolated (it's an addition in Layout to hydrate Header with a nested menu component, Cart). The packages are the exact same as the template, except Elder which is at 1.1.8 because 1.1.7 was producing the same issues. I have tried Node versions 12 and 14. npm start loses styles on the nested component Cart. let cartOpened = process.env.componentType === 'server' is a valid workaround.

Kind of sad they're dropping sourcemap support in the Svelte plugin but I guess there's not enough demand to deter them from doing so, which is understandable if that's the case.

@s-kris
Copy link

s-kris commented Dec 26, 2020

I can confirm the same issue.

@nickreese
Copy link
Contributor

nickreese commented Dec 26, 2020

@s-kris make sure you’re on the latest release and have rerun rollup.

@s-kris
Copy link

s-kris commented Dec 26, 2020

@nickreese Thank you :)
it worked now! I manually changed the version to 1.2.4

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