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

Compatibility with preact #588

Closed
piotr-cz opened this issue Feb 28, 2021 · 24 comments
Closed

Compatibility with preact #588

piotr-cz opened this issue Feb 28, 2021 · 24 comments
Labels
bug Something isn't working

Comments

@piotr-cz
Copy link
Contributor

piotr-cz commented Feb 28, 2021

React version (preact 10.5.12)

Concurrent mode no

Rest Hooks version (e.g., 5.0.6)

Describe the bug (This is rather a feature request but IMO it's more appropriate to use bug template)
This package seems to not work with Preact (alternative to React with compatible API).

Problem comes up when wrapping an application with <CacheProvider>.
Engine stops rendering any children and what's unfortunate doesn't show any errors.

Small hint is that ot does render when I remove context providers and keep just {children} in
https://github.com/coinbase/rest-hooks/blob/e1e353dfc64725c79ab99bb6a0c85114399c6dfc/packages/core/src/react-integration/provider/CacheProvider.tsx#L56-L64

To Reproduce
Steps to reproduce the behavior:

  1. Create new codesandbox with Preact template
  2. Add @rest-hooks/rest & rest-hooks to dependencies
  3. Add import { CacheProvider } from 'rest-hooks'; to src/index.js
  4. Wrap App by CacheProvider
  5. Problem: Rendered output disappears

Or check codesandbox example

Expected behavior
Package works with Preact just as it would with React

Additional context
No console errors.

@piotr-cz piotr-cz added the bug Something isn't working label Feb 28, 2021
@ntucker
Copy link
Collaborator

ntucker commented Feb 28, 2021

Thanks for your interest!

Rest Hooks imports createContext from React itself (listed as a peerDep). Seems like you would need to tell your build system to change all react imports to preact imports?

Would love addition of a unit test as well as docs instructions for usage with preact. I'm happy to fix any reasonable compatibility to pass said test.

@piotr-cz
Copy link
Contributor Author

Build system I'm using (WMR/ Rollup) is already configured to alias react to preact/compat.
Same in the codesandbox example, although it uses webpack.

I'm using SWR in same setup without any issues even that it's also using react as peer dependency.

Issue must come from somewhere else.

@ntucker
Copy link
Collaborator

ntucker commented Feb 28, 2021

I downloaded a zip of the codesandbox, when i run yarn start I get this:

./~/@rest-hooks/core/lib/react-integration/hooks/useResetter.js
7:17-27 "export 'useContext' was not found in 'react'
./~/@rest-hooks/core/lib/react-integration/hooks/useResetter.js
8:24-35 "export 'useCallback' was not found in 'react'
./~/@rest-hooks/core/lib/react-integration/hooks/useFetchDispatcher.js
9:17-27 "export 'useContext' was not found in 'react'
./~/@rest-hooks/core/lib/react-integration/hooks/useFetchDispatcher.js
10:24-35 "export 'useCallback' was not found in 'react'
./~/@rest-hooks/core/lib/react-integration/hooks/useInvalidateDispatcher.js
7:17-27 "export 'useContext' was not found in 'react'
./~/@rest-hooks/core/lib/react-integration/hooks/useInvalidateDispatcher.js
8:29-40 "export 'useCallback' was not found in 'react'

Is there something weird with the tooling maybe?

@ntucker
Copy link
Collaborator

ntucker commented Feb 28, 2021

I don't find useContext anywhere in the node_modules of preact/compat. Are hooks only supported in a beta version of preact?

@ntucker
Copy link
Collaborator

ntucker commented Feb 28, 2021

According to https://www.npmjs.com/package/preact-compat - it's only for outdated versions of preact, which is probably why it doesn't have hooks.

For Preact X, please uninstall preact-compat and replace your aliases with preact/compat.

The config in codesandbox has:

    "moduleNameMapper": {
      "\\.(jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2|mp4|webm|wav|mp3|m4a|aac|oga)$": "<rootDir>/src/tests/__mocks__/fileMock.js",
      "\\.(css|less|scss)$": "identity-obj-proxy",
      "^./style$": "identity-obj-proxy",
      "^preact$": "<rootDir>/node_modules/preact/dist/preact.min.js",
      "^react$": "preact-compat",
      "^react-dom$": "preact-compat",
      "^create-react-class$": "preact-compat/lib/create-react-class",
      "^react-addons-css-transition-group$": "preact-css-transition-group"
    }

@piotr-cz
Copy link
Contributor Author

@ntucker
Copy link
Collaborator

ntucker commented Feb 28, 2021

Right, but preact-compat is version 8, you need to have preact/compat instead of preact-compat

@piotr-cz
Copy link
Contributor Author

Ah, seems that the codesandbox template ships with outdated packages

@ntucker
Copy link
Collaborator

ntucker commented Feb 28, 2021

Got the codesandbox to work by doing two things:

  • upgradeing the cli from 1 to 3
  • targetting an actual dom node in render render(<My />, document.body);

'root' wasn't actually on the page

@piotr-cz
Copy link
Contributor Author

Yeah, I've done the same in meantime and it does build and render now.
That would mean the problem is when using Rollup instead of Webpack.

I'll come back to that tomorrow.
Thank you for taking time to look at it. At least we know react-hooks may work with preact.

@ntucker
Copy link
Collaborator

ntucker commented Mar 1, 2021

Gonna close this for now since it seems to work. Would love any docs contributions on getting start with the library in Preact! :)

@ntucker ntucker closed this as completed Mar 1, 2021
@piotr-cz
Copy link
Contributor Author

piotr-cz commented Mar 3, 2021

I'm sorry, the issue is not resolved.

Compatiblity with Preact may be achieved only in projects created with preact-cli (I assume it's compiling vendor packages with own babel config and does other tweaks).

I've prepared issue reproduction project here: preact-rollup-react-jsx-transforms.

Basically there are two issues:

  1. rest-hooks uses global variables (global, process) which may be added by webpack based pipelines - but it's not secure to rely on that
  2. when Babel isn't configured to use babel-plugin-transform-react-jsx plugin, it falls back to it's jsx runtime helper which works only with react.
    See reactjs > Introducing the New JSX Transform

@ljharb
Copy link
Contributor

ljharb commented Mar 3, 2021

It’s quite secure to rely on node globals - a node module bundler that does not provide them is broken.

@piotr-cz
Copy link
Contributor Author

piotr-cz commented Mar 3, 2021

@ljharb
What is the reason to use global.requestIdleCallback instead of window.requestIdleCallback?

@piotr-cz

This comment has been minimized.

@ntucker
Copy link
Collaborator

ntucker commented Mar 3, 2021

@ljharb
What is the reason to use global.requestIdleCallback instead of window.requestIdleCallback?

node doesn't have window. I believe globalThis is now the canonical way of referring to such things, so perhaps this should change. This would break older integrations however as globalThis is somewhat new.

@piotr-cz can you open a new issue to track these new things? Also I'm a bit confused about which usage of the jsx transform will work with preact. I'm hoping it's the new version as I hope to use that in the future.

@ljharb
Copy link
Contributor

ljharb commented Mar 3, 2021

You can use https://npmjs.com/globalThis to maintain support, but indeed, global is the proper thing to use, and webpack 5 has intentionally broken its core use case by not doing this by default.

@ntucker
Copy link
Collaborator

ntucker commented Mar 3, 2021

@ljharb How might I include the https://www.npmjs.com/package/globalthis shim only for legacy cases, as to not bloat code size?

@ljharb
Copy link
Contributor

ljharb commented Mar 3, 2021

I suppose you could do a conditional require, but generally speaking you can't avoid code size increases when maintaining support for older versions of things.

In this case, requestIdleCallback is already a global variable when present, so const rIC = typeof requestIdleCallback === 'function' && requestIdleCallback; would handle it with no direct reference to the global.

@ntucker
Copy link
Collaborator

ntucker commented Mar 3, 2021

Hmm, does const rIC = typeof requestIdleCallback === 'function' && requestIdleCallback; work in React Native? Trying to figure out why I did it this way

@ljharb
Copy link
Contributor

ljharb commented Mar 3, 2021

If it's a global variable there, it should.

The only reason I can think of to use global. is because you ran into the ReferenceError in node, and that's one way to avoid it. The typeof is another.

@ntucker
Copy link
Collaborator

ntucker commented Mar 3, 2021

Ah, I now realize it's because microsoft/TypeScript#40807. I can just add the libdef myself so this won't be a problem.

@ntucker
Copy link
Collaborator

ntucker commented Mar 3, 2021

@piotr-cz When using rollup you can define process via https://github.com/coinbase/rest-hooks/blob/master/packages/rest-hooks/rollup.config.js#L44

Though I don't think rollup is intended for actual projects, but just to build libraries.

@piotr-cz
Copy link
Contributor Author

piotr-cz commented Mar 4, 2021

@ntucker
I'll open a pull request that will address the jsx transforms for preact issue.
If you are interested, I've written an explanation here: https://github.com/piotr-cz/preact-rollup-react-jsx-transforms#components-wrapped-in-vendor-package-component-dont-render

For handling globals, it's fairly easy to replace them as you noted.

Actually I'm using Rollup as a web app bundler for few years.
I guess it's also used for svetle, snowpack and wmr. I never liked webpack to be honest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants