search

natemoo-re / microsite

Do more with less JavaScript. Microsite is a smarter, performance-obsessed static site generator powered by Preact and Snowpack.
https://npm.im/microsite
MIT License
877 stars 16 forks source link

Hydration fails when a component is used on multiple pages #96

Closed eyelidlessness closed 3 years ago

eyelidlessness commented 3 years ago

Version: 1.1.0-next.3

Steps to reproduce

  1. Render a hydrated component on multiple pages
  2. Build/serve
  3. Load in browser

Expected behavior

The component will hydrate on each page.

Observed behavior

No JS is loaded at all.

Apparent cause

Any shared component with hydration is bundled as a shared chunk, but the shared chunk is excluded from hydrateBindings, and doesn’t appear to be referenced anywhere else. If I comment out that check it does hydrate as expected.

I was tempted to offer a PR removing that check, but I can’t shake the feeling that there’s a reason it’s there and the intent was to handle that case differently.

Related side note: in terms of contribution, my usual approach is to write a failing test exercising my bug, then fix to make the test pass. But at present Microsite doesn’t appear to have a test suite. Is that something you’d be interested in? If so do you have a preferred test framework? I’d be happy to look into setting up CI for any tests I contribute, and to try to increase coverage as I contribute.

eyelidlessness commented 3 years ago

I’ll add that it’s entirely possible something strange in my setup is causing this, but I’ve tried to eliminate that possibility (and to find workarounds) before reporting. But so far both efforts have come up empty.

natemoo-re commented 3 years ago

Hmm, I'll have to look into this. You've definitely diagnosed the problem correctly, I don't think it's anything with your setup. I'm trying to remember if there was a reason I was ignoring the shared chunk in hydrateBindings...

As for the tests, yes please! 🎉 Microsite desperately needs tests. I was moving fast and breaking things (and also didn't expect this gain traction) so I skipped them. I think uvu would be a good fit, and your PR can start small. Any help with CI would much appreciated!

eyelidlessness commented 3 years ago

Hmm, I'll have to look into this. You've definitely diagnosed the problem correctly, I don't think it's anything with your setup. I'm trying to remember if there was a reason I was ignoring the shared chunk in hydrateBindings...

If your thinking is anything like mine, I wouldn’t be surprised if the “eventually” part of the comment became an implicit TODO and the naive solution got sidetracked haha. FWIW I did seem to notice other “shared” stuff that wasn’t hydration related getting picked up, though that is definitely something that might be more related to my setup because I’m fairly certain it was some junk from my Fela wrapper library. But it’s possible that shared chunk is a potential bloat. If you haven’t knocked it out before me I’ll look into this more tomorrow with fresh eyes and 🥳 tests!

As for the tests, yes please! 🎉 Microsite desperately needs tests. I was moving fast and breaking things (and also didn't expect this gain traction) so I skipped them. I think uvu would be a good fit, and your PR can start small. Any help with CI would much appreciated!

Oh I hadn’t heard of this one. I’m so so glad you didn’t say Jest (though that’s the only one I know really well). I’ll give that a look tomorrow too. FWIW I’m very surprised to see Ava benchmark so poorly in their writeup, in my (admittedly exploratory) experience it’s been nothing but snappy. This is definitely gonna get me thinking about my own testing preferences, although I’m torn between their respective APIs and feature sets.

I have a few more test related questions some of which I’m going to copy/adapt from another project I offered to bring tests

which I’m ashamed to admit I dropped the ball on due to major burnout and a resulting career change, which is a big part of why I’m building a personal site now, [link for accountability][1]. I do want to come back to that project when I’ve got other stuff in order but right now I’m not even doing anything that uses a database.

but anyway...:

eyelidlessness commented 3 years ago

Oops I meant to add more on the refactor topic. There are definitely cases where I see opportunities here. If I were to try anything big I’d definitely want to open a discussion issue/chat first. Most prominent are (as mentioned) anywhere I see any types, and some functions in the build flow that are quite large which can be hard to follow. I’m definitely not looking to come in and throw weight around, but this is something I inevitably encounter in new-to-me projects and I find it helpful to ask upfront on other people’s projects (and I tend to be imperious when they’re mine, so I understand any fraction of same).

eyelidlessness commented 3 years ago

I should also add, part of the reason for all the big upfront questions (it’s okay if you’re squishy on any of them btw) is because I want to make sure I’m being a good citizen/visitor when I contribute, especially if I’m introducing anything new. And also because I see a lot of potential here and want to take it as seriously as I would take starting on a new team with a project I’m gonna be living with and wake up thinking about (which no joke I did today, with my dev setup so close I can almost taste it). Just saying, I’m sorry again for blowing up your notifications but I’m seriously excited to work with this project.

natemoo-re commented 3 years ago

Thanks so much for the enthusiasm—I'm looking forward to having some help on this project!

I'm pretty open to whatever your preferences are on testing, honestly.

  • Are you interested in type-level testing?

I don't have any experience in this area, but I'm not sure how useful type-level testing would be since the codebase is already written in TS. We can consider this later if the need surfaces.

  • You’re already using GitHub Actions, is that what you’d prefer for CI?

GitHub Actions would be great—happy to lend a hand here.

  • I see you’re using prettier. Do you have other code style preferences you’d like me to know about upfront that I can’t pick up from the current code? (I really try hard to conform on formatting, it’s really distracting from more meaningful review.)

Nope! I try to focus on productivity more than nitpicking code style. You already seem familiar with the code base so I'm not concerned about it.

  • Do you have a preference for ratio of unit tests (mocking/stubbing dependencies where necessary or appropriate) vs integration tests vs snapshot tests (this is new to me, they literally didn’t exist in a meaningful way the last time I did any really involved FE work)?

The main reason I hadn't tackled tests up until now is because I had no idea how to approach integration or snapshot tests. It's not something I do much of, but I know those would be the most useful. Tbh anything is going to be better than my current (very manual) process.

  • You already mentioned small PRs, should I assume that’s your preference for commits too? (I do too but I want to make sure I don’t get into a rabbit hole and offer a big inscrutable commit or you’re going to find it problematic)

Eh, small commits are easier to grok but I realize that some tasks (like the initial testing setup) will involve a lot of changes, so I don't have a problem if there are some big ones.

  • Do you prefer tests next to modules (eg foo.ts/foo.test.ts) or a separate root directory for tests that mirrors test modules (or something else)?

Separate directory within each module works for me

packages
  - microsite
    - src/foo.ts
    - test/foo.test.ts
  • Do you prefer BDD-style test descriptions or more technical behavior (eg test “it does some behavior users expect” vs “test correct frobnication”)?

I like BDD!

  • If I spot things that smell like “move fast and break stuff” (eg types are any or code flow is hard to understand) would you welcome refactors? If so I’ll try to keep them as isolated as possible in commits, and as small/directly related to more directly relevant work.

Totally open to refactors! I'd love the codebase to be as easy to approach as possible.

natemoo-re commented 3 years ago

@eyelidlessness I invited you to this repo. Feel free to make branches and PRs here instead of on a fork, please just add me as a reviewer on any PRs!

eyelidlessness commented 3 years ago

Wow thank you! Gonna start digging into tests for this issue!

eyelidlessness commented 3 years ago

Following up for EOD because it’s puppy time. I’ve had a heck of a day trying to get testing setup. It’s actually not that bad for single test runs in Uvu (and I even have that working with VSCode debugging), I just made a series of dumb mistakes like not stopping to ask for directions (I missed that Uvu requires an explicit call to run) and not taking breaks.

But that’s all working and if I can get a few minutes of focus at the keyboard tonight I’ll push it up in a branch, otherwise first step tomorrow.

The other place my day went off the rails is a fairly typical one for me. Uvu doesn’t have a watch mode, which is pretty much my entire dev flow: start up test runner in watch/debug mode, red-green-refactor. So I did the stupid things I’d normally do:

  1. Use ts-node and whatever watcher I can find.
  2. Goodness this is slow.
  3. Try to reverse-engineer and brute force.
  4. Don’t give up.

I’m reporting failure on that front. These tools don’t like each other. And I’ve learned yet more about the hell that ESM/CJS interop can be. However! Once I got the nerve to take a break I also stopped to ask for more directions. And I think there’s a good chance Estrella is a potential solution here. It’s not its primary use case but it has watch mode built in, it’s using esbuild, it’s designed for customization, and it has a nifty CLI-first interface. So I’ll be looking at this tomorrow (absent objections).

I was really hoping to get further on this front today, but honestly every day since I’ve returned to frontend dev has felt like that.

While I’m here, I know the license allows it but I’d like to ask explicit permission to push out next builds (namespaced to me on NPM so it’s clear they’re not official cuts unless you’d prefer otherwise) so I can also evaluate them on my own project without doing the horrific npm/yarn link dance. Assuming I even get so far as that. But I’m simultaneously tracking contribution here as well as my own site and I’m so close to being able to build my site I can taste it, and trying to do everything I can to get out of my own way as possible on that.

natemoo-re commented 3 years ago

@eyelidlessness Awesome, thanks for the update! I'm not necessarily tied to uvu, so if you decide it's more trouble than it's worth, feel free to switch to whatever works for you. I didn't realize uvu didn't have a watcher built in, that sounds like a pain. Estrella does seems cool, though, if you want to give that a shot!

Feel free to put namespaced builds out if you'd like! The link dance is very frustrating. To work around that locally (though this is not much better), I've been running npx lerna exec --scope microsite -- npm pack and then directly installing the generated .tgz file by running npm i ../microsite/packages/microsite/microsite-1.1.0-next.3.tgz. Whatever works for you, though!

eyelidlessness commented 3 years ago

Great news! I got a little proof of concept with Estrella working, and goodness it is fast! And it took very little fuss to get going. I'm going to clean up some mess, setup a GitHub Action, and push up a PR. It's not a huge diff, but I think I'd like to keep test setup separate from fixing this issue to get a meaningful win. Hopefully ready within the hour!

natemoo-re commented 3 years ago

@eyelidlessness I remembered the reasoning here—I had assumed that components in the _shared chunk were shared between other components, and therefore would never be top-level component needed in hydrateBindings. Clearly that assumption doesn't hold, so definitely want to fix this one.

eyelidlessness commented 3 years ago

It’s entirely possible I was misunderstanding something about intended usage! I was just validating that I could hydrate a dumb counter component, and include it on multiple pages. Is there some additional ceremony required for that? Or is that what you mean by “that assumption doesn’t hold”?

For real-world use I definitely intend to share an interactive component across pages, for instance a dark mode toggle.

eyelidlessness commented 3 years ago

Without getting too hung up on the big testing question, I strongly suspect the resolution of this is pretty simple:

  1. Shared hydration chunks and shared static chonks should go to different manual chunks
  2. Shared hydration chunks should go to the bundle
  3. Eventually the implicit TODO should be addressed and allow more granular code splitting
  4. For my own use I’m gonna want to bundle up everything headed to the client and un-split but that’s obviously not a requirement here. Just stating my use case!
eyelidlessness commented 3 years ago

I can confirm that these proposed changes do include my expected hydration behavior, and that moving/excluding shared static content does prevent the shared chunk bloat I saw before. I need to run some errands but I’ll have a small PR for this hopefully this afternoon/evening.

eyelidlessness commented 3 years ago

Oh I wanted to mention, I originally tried branching off #104 to make these changes, but builds in my project failed due to the removed .js extensions on imports. Another cause for concern 😕