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.
I certainly see the value, and do think we need to set up something to facilitate that better (I'll be looking into it). But there's a fair number of issues in this approach, which is why I'm fairly apprehensive about encouraging it. Anything that uses the plugin API (outside of this repo, anyways) won't function correctly.
There was a problem hiding this comment.
We could just do what I did initially and make a react-hooks file with these implementations
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.
fix #4457
cc @marvinhagemeister