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

Web-only app breaks #2660

Closed
nandorojo opened this issue Oct 27, 2023 · 7 comments · Fixed by #2666
Closed

Web-only app breaks #2660

nandorojo opened this issue Oct 27, 2023 · 7 comments · Fixed by #2666

Comments

@nandorojo
Copy link
Contributor

Description

Screenshot 2023-10-27 at 2 47 15 PM

react-native isn't used on Web. This check should be moved to a native-only file.

Steps to reproduce

Create a web-only project that imports RNGH.

Snack or a link to a repository

https://stackblitz.com/edit/nextjs-tcj82d?file=pages%2Findex.tsx,package.json,next.config.js,rn-pkc.json

Gesture Handler version

2.13.4

React Native version

N/A

Platforms

Web

JavaScript runtime

None

Workflow

None

Architecture

None

Build type

None

Device

None

Device model

No response

Acknowledgements

Yes

@github-actions
Copy link

Hey! 👋

The issue doesn't seem to contain a minimal reproduction.

Could you provide a snack or a link to a GitHub repository under your username that reproduces the problem?

@bartlomiejbloniarz
Copy link

@nandorojo shouldn't react-native-gesture-handler be added to the transpiled packages list in next.config.js?

module.exports = withExpo({
  /** @type {import('next').NextConfig} */
  transpilePackages: [
    'moti',
    'react-native-reanimated',
    'react-native',
    'react-native-gesture-handler',
  ],
});

@nandorojo
Copy link
Contributor Author

Yes, but the version you're going to get from this package isn't going to be useful. It'll be the RNW version. So I think it would make sense to move this to a native-only file either way.

@nandorojo
Copy link
Contributor Author

I should have clarified in the original post that checking the version, even when the error is silenced, doesn't make much sense for web.

@m-bert
Copy link
Contributor

m-bert commented Oct 31, 2023

Hi @nandorojo! IIRC React Native version is not considered on web. REACT_NATIVE_VERSION is used only inside GestureDetector, in code section that is not executed on web (see this line for context).

@chriscoomber
Copy link
Contributor

I have the exact same error, and I am adding 'react-native-gesture-handler' to transpilePackages already - that doesn't appear to fix the error. (Indeed, I tried it on a fork of @nandorojo's repro and it didn't fix the error there.) So I think this is still a bug, unless I'm missing something.

The error goes away if you install react-native in addition to react-native-web in your app, not that I'm suggesting that's a sane workaround.

Would it be possible to split utils.ts into utils.ts and utils.web.ts (with the latter containing a dummy implementation which doesn't attempt to import react-native/package.json)?

@nandorojo
Copy link
Contributor Author

Yeah this would make the most sense.

The reason is because the RNW resolution from Webpack doesn't work for .json files. So importing from react-native/package.json does not map to react-native-web/package.json

This code should be moved to a native-only file if web compatibility is a relevant feature.

chriscoomber pushed a commit to chriscoomber/react-native-gesture-handler that referenced this issue Nov 3, 2023
m-bert pushed a commit that referenced this issue Nov 6, 2023
## Description

Fixes #2660

Depending on this package in a web-only app is causing a compile error.
This is due to this package importing `react-native/package.json`, which
for web-only builds doesn't exist. This import is only done to obtain
the react-native version number, which isn't used for web anyway. So it
seems easy enough to move this version number to a native-only file.

I chose to export a **function** so that it's a _logic error_ to use
`getReactNativeVersion` on web, and an error is thrown (right now it's
only used in one place, behind a check that makes sure it runs on native
only). It might have been easier to just export a **const**, with fake
values on web, but that would hide the error for a potentially more
confusing error down the line.

## Test plan

Not tested, sorry. Would appreciate if someone could help out here.

---------

Co-authored-by: Chris Coomber <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants