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

React package breaks HMR / fast refresh #250

Open
souporserious opened this issue Oct 23, 2022 · 9 comments
Open

React package breaks HMR / fast refresh #250

souporserious opened this issue Oct 23, 2022 · 9 comments
Labels

Comments

@souporserious
Copy link

souporserious commented Oct 23, 2022

When importing @preact/signals-react into a Next.js app (and possibly other frameworks) it breaks fast refresh in local development. I've created a simple reproduction here that showcases the problem. My guess is the overriding of the jsx/s methods is causing this. I know this was to avoid a compilation step, but it might be nice to offer configuration here for setting a pragma to do this when working with bundlers.

Pragma as a possible solution

Exposing the jsx function through a specific import and setting a pragma explicitly like Emotion does could be a nice solution:

/** @jsx jsx */
import { jsx } from '@preact/signals-react/jsx'

This would avoid overriding the global jsx methods when desired, could be configured in bundlers for global use, and would be a non-breaking change.

Happy to help out with a PR for this if it sounds like a reasonable fix 😊

@souporserious
Copy link
Author

souporserious commented Oct 23, 2022

Having trouble updating my reproduction to showcase, but I was able to confirm that the jsx override is indeed the cause of the fast refresh issue.

@jeremy-coleman
Copy link

I noticed this yesterday in vite. I took the same app and made a version using solidjs, preact with signals and react with signals. Only the react app’s hmr broke. preact and solid worked fine. I didnt look into it, glad u did tho. Maybe fixable by configuring jsxImportSource ?

@souporserious
Copy link
Author

@jeremy-coleman yeah, ideally this could be fixed through jsxImportSource!

Some updates from research I did today.. I tried moving to a pragma, but keep getting errors because store.updater._start is undefined. If I comment it out it still doesn't seem to work, so not sure how feasible a pragma solution is. I might just not know enough details here to be setting things up right 😇

I did test the previous version (1.1.1) before #219 and fast refresh was working prior. Maybe there's a way to both have useExternalStore and tap into React internals again to restore this functionality?

@rschristian rschristian changed the title react package breaks NextJS fast refresh React package breaks HMR / fast refresh Nov 10, 2022
@souporserious
Copy link
Author

souporserious commented Feb 12, 2023

Revisited this again and ended up using my own React wrapper and pragma which seems to work well and not break HMR. Note, this doesn't set up useSyncExternalStore so it could have trouble with React Suspense. It's basically a trimmed version of the @preact/signals-react package:

/** A wrapper component that renders a Signal's value directly as a text node. */
export function Text({ data }: { data: Signal }) {
  const [value, setValue] = React.useState(data.value)

  useSignalEffect(() => setValue(data.value))

  return value
}

/** Jsx function to replace React.createElement calls with the ability to render signals as text nodes. */
export function jsxs(
  type: React.ElementType<any>,
  { children, ...props }: { children?: React.ReactNode } & Record<string, any>,
) {
  return React.createElement(type, {
    children: Array.isArray(children)
      ? React.Children.toArray(children?.map(signalToText))
      : signalToText(children),
    ...props,
  })
}

@freerror
Copy link

freerror commented Feb 25, 2023

If it helps anyone, I have this issue with @vitejs/plugin-react-swc but if I switch my project to use @vitejs/plugin-react instead, HMR/fast reloading starts working again.

You can do this by installing the @vitejs/plugin-react npm package, and simply changing the import to import react from '@vitejs/plugin-react' in your vite.config.ts.

As I'd like to use SWC I am looking for a better solution.

@lsimone
Copy link

lsimone commented Mar 10, 2023

When importing @preact/signals-react into a NextJS app (and possibly other frameworks) it breaks fast refresh in local development. I've created a simple reproduction here that showcases the problem. My guess is the overriding of the jsx/s methods is causing this. I know this was to avoid a compilation step, but it might be nice to offer configuration here for setting a pragma to do this when working with bundlers.

Pragma as a possible solution

Exposing the jsx function through a specific import and setting a pragma explicitly like Emotion does could be a nice solution:

/** @jsx jsx */
import { jsx } from '@preact/signals-react/jsx'

This would avoid overriding the global jsx methods when desired, could be configured in bundlers for global use, and would be a non-breaking change.

Happy to help out with a PR for this if it sounds like a reasonable fix blush

I am facing the same issue with next v12.3.1 and working without Fast refresh is a huge degradation of devExp, so I hope this will be solved soon (or at least a workaround is found), otherwise I think I will have to do without signals, even if I really appreciate this API.

I am not really aware of the logic of this package that provokes this issue, but If I can help (even with testing) I would be glad to.

@luisgrases
Copy link

Having this same issue on next v13, HMR is not working

@jackvane
Copy link

Any sign of this getting fixed anytime soon? Signals feels like a great optimization but if it the cost is DX, it makes it hard to opt into.

@sbesh91
Copy link
Sponsor

sbesh91 commented Aug 31, 2023

If you follow this guide to set up the new babel transform for Signals, you should be all set!
https://github.com/preactjs/signals/tree/main/packages/react-transform
We only had to deviate slightly. The name of the babel plugin was getting auto-normalized, so your plugin name should look like this "module:@preact/signals-react-transform" for now.

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

No branches or pull requests

8 participants