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.

Sign up for GitHub

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

Jump to bottom

fix demo build #4458

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+2 −3
Open

fix demo build #4458

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
43c1e67
fix demo build
gengjiawen Jul 27, 2024
0cce45b
Update demo/index.jsx
gengjiawen Jul 27, 2024
6d858a2
add build demo to CI
gengjiawen Jul 27, 2024
9d947b2
revert npm audit
gengjiawen Jul 27, 2024
4d1461e
delete ci check for demo
gengjiawen Jul 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions compat/src/render.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import {
} from 'preact/hooks';
import {
useDeferredValue,
useInsertionEffect,
useSyncExternalStore,
useTransition
} from './index';
Expand Down Expand Up @@ -297,7 +296,7 @@ export const __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED = {
useEffect,
useId,
useImperativeHandle,
useInsertionEffect,
useInsertionEffect: useLayoutEffect,
Comment on lines -300 to +299
Copy link
Member

@rschristian rschristian Jul 27, 2024 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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!

useLayoutEffect,
useMemo,
// useMutableSource, // experimental-only and replaced by uSES, likely not worth supporting
Expand Down
4 changes: 2 additions & 2 deletions demo/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import NestedSuspenseBug from './nested-suspense';
import Contenteditable from './contenteditable';
import { MobXDemo } from './mobx';
import Zustand from './zustand';
import ReduxToolkit from './redux_toolkit';
import ReduxToolkit from './redux-toolkit';

let isBenchmark = /(\/spiral|\/pythagoras|[#&]bench)/g.test(
window.location.href
Expand Down Expand Up @@ -140,7 +140,7 @@ class App extends Component {
<Link href="/zustand" activeClassName="active">
zustand
</Link>
<Link href="/redux-toolkit" activeClassName="active">
<Link href="/demo/redux-toolkit" activeClassName="active">
gengjiawen marked this conversation as resolved.
Show resolved Hide resolved
redux-toolkit
</Link>
</nav>
Expand Down
Loading
Loading