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

fix demo build #4458

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix demo build #4458

wants to merge 5 commits into from

Conversation

gengjiawen
Copy link
Contributor

@gengjiawen gengjiawen commented Jul 27, 2024

@coveralls
Copy link

coveralls commented Jul 27, 2024

Coverage Status

coverage: 99.485% (-0.001%) from 99.486%
when pulling 4d1461e on gengjiawen:fix/demo
into 9351588 on preactjs:main.

@rschristian
Copy link
Member

Yeah unfortunately there's a circular reference there that Vite can't work out from the source files, will need to use the built.

@gengjiawen
Copy link
Contributor Author

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.

demo/index.jsx Outdated Show resolved Hide resolved
demo/package-lock.json Outdated Show resolved Hide resolved
Co-authored-by: Ryan Christian <[email protected]>
Comment on lines -300 to +299
useInsertionEffect,
useInsertionEffect: useLayoutEffect,
Copy link
Member

@rschristian rschristian Jul 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not split that alias across two modules. Especially not for the demo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will make the vite dev and build work.

Copy link
Member

@rschristian rschristian Jul 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@JoviDeCroock JoviDeCroock Jul 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@rschristian rschristian Jul 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

@rschristian rschristian Jul 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rschristian: Good to know -- thank you!

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

Successfully merging this pull request may close these issues.

preact demo won't run
6 participants