fix demo build #4458
fix demo build #4458
Conversation
|
Yeah unfortunately there's a circular reference there that Vite can't work out from the source files, will need to use the built. |
looks fixable via a hack. |
Co-authored-by: Ryan Christian <[email protected]>
| useInsertionEffect, | ||
| useInsertionEffect: useLayoutEffect, |
There was a problem hiding this comment.
I'd rather not split that alias across two modules. Especially not for the demo.
There was a problem hiding this comment.
It will make the vite dev and build work.
There was a problem hiding this comment.
And I get that, but the demo is very minor (I don't think we even use it for anything -- frankly, I'd be up for removal as it's a bit of a mess) and shouldn't change source.
The issue is that if we do provide an implementation for useInsertionEffect, now there's two separate places that need to be updated which I don't love. Having Vite use built modules is a better solution anyhow as from source is not representative.
There was a problem hiding this comment.
And I get that, but the demo is very minor
I really like it since it very easy to tinker with preact source. Anyway, I will leave you guys decide with PR.
There was a problem hiding this comment.
I mean, let's find a different solution that doesn't involve changes to our packages 😅 because at this point it points at a bug in vite i.e. maybe upgrade vite instead
There was a problem hiding this comment.
@JoviDeCroock Not necessarily a bug, but a cyclical reference. index -> render, render -> index. I think Microbundle/rollup does throw warnings because of this (but it can handle it alright as it's just bundling). Vite, serving the unbundled individual modules in the browser, falls over (but again, non-representative as no one is running Preact from source in reality, so...)
Edit: Yeah, not a bad idea. I might go through tomorrow and see if I can clean out all cyclical references, as they're probably not ideal anyhow.
There was a problem hiding this comment.
Took another look: while the compat issue can be reasonably fixed by externalizing the React hook implementations to a new module as mentioned, core has a couple cyclical references that I don't think can be untangled without merging modules or doing an internal barrel file sorta thing. I don't think either are a worthwhile approach.
Best solution is to insert "preact": "../" (or something along those lines) into /demo/package.json so that it uses built output instead.
There was a problem hiding this comment.
no one is running Preact from source in reality
FWIW, I just did (I wanted a no-build workflow that wasn't minified for development, so I can get familiar with the Preact source during dev). Firefox & Chrome both choked on this cyclical reference. This change, plus adding .js extensions to the imports, was all I've needed so far to run Preact from source (IMO it's worth the effort).
There was a problem hiding this comment.
FWIW, I just did (I wanted a no-build workflow that wasn't minified for development, so I can get familiar with the Preact source during dev).
There certainly are some situations where this works, with that being one of them, however, this breaks expectations of any lib using the plugin API (which at least the first-party ecosystem heavily, heavily relies upon). As such, we don't recommend it in prod as it'll lead to nasty surprises in all likelihood.
There was a problem hiding this comment.
@rschristian: Good to know -- thank you!
fix #4457
cc @marvinhagemeister