search

software-mansion / react-native-gesture-handler

Declarative API exposing platform native touch and gesture system to React Native.
https://docs.swmansion.com/react-native-gesture-handler/
MIT License
5.85k stars 954 forks source link

fix: web compatibility #406

Closed jaulz closed 5 years ago

jaulz commented 5 years ago

The motivation for me was to use react-navigation@^3 and I stumbled across quite a few missing imports. This PR will introduce some shims so it shouldn't be a problem anymore.

osdnk commented 5 years ago

@jaulz Hi, thanks for you PR. I believe it's a crucial step for RNGH! Is it WIP? If yes, please add proper label?

jaulz commented 5 years ago

@osdnk sorry, I found some issues with the original commit and pushed a fix now 😊As requested I added some comments and it should be in a state where you could please take a look. I tried to touch the existing files at a minimum and also cross checked on native devices if it still works as expected.

miracle2k commented 5 years ago

Sorry for the interjection:

I was trying this PR, but the thing I run into is that my babel transforms the access to this in the static property in this line (https://github.com/kmagiera/react-native-gesture-handler/blob/master/touchables/GenericTouchable.js#L56) into

GenericTouchable2.propTypes = _objectSpread({}, (void 0).internalPropTypes, (void 0).publicPropTypes);

(i.e., it replaces this with undefined).

Clearly something is wrong with the way I have setup my babel for the react-native-web compile. Can @jaulz or someone else share a working babel config?

necolas commented 5 years ago

That just looks like incorrect code. this should be the class name instead

miracle2k commented 5 years ago

@necolas Funny. If I change those lines to 

static propTypes = {
    ... GenericTouchable.internalPropTypes,
    ... GenericTouchable.publicPropTypes,
  };

It compiles with webpack, but then react-native@0.57.8 in the iOS emulator says:

Cannot read property 'internalPropTypes' of undefined

The exact opposite behaviour.

miracle2k commented 5 years ago

I am fairly certain this is related to the Hot Module Reloading transform inserted by metro here by merging into babelrc this partial config.

Transforming the file in question with babel and using only metro-react-native-babel-preset, the output is sensible: https://gist.github.com/miracle2k/3bc7f1c50080397dbf0d92cfb3101677#file-generictouchable-babel-with-metro-preset-js.

But the actual code generated by react-native for the iOS emulator looks like this: https://gist.github.com/miracle2k/3bc7f1c50080397dbf0d92cfb3101677#file-generictouchable-rn-compilation-js Notice the _wrapComponent calls which come from the react-transform plugin: https://github.com/facebook/metro/blob/master/packages/metro-babel7-plugin-react-transform/src/index.js#L420.

The problem goes away if I manually edit the file linked above (reactNativeTransformer.js#L131) to ensure the transform is not added. Then, like @necolas says, using the classname instead of this works. Note that the current react-native ships with an old metro version, v0.48.5, so the problem may or may not be fixed in a newer one.

I can see that @osdnk added this code quite recently. Maybe as a workaround for this issue we could rewrite that part and pull the re-used proptypes into the global module scope.

osdnk commented 5 years ago

I will merge it if needed.

jaulz commented 5 years ago

@osdnk @brentvatne it would be great if could you have another look. I assume we could even implement a proper PanGestureHandler with the help of PanResponder from react-native-web but this might be something for the future.

alidcast commented 5 years ago

@osdnk @jaulz anything remaining in this PR?

jaulz commented 5 years ago

@alidcastano I am waiting for @osdnk feedback on this but I would love to see this being merged since it would solve a big pain point for a lot of react-native-web users right now. Once we have it we should consider the actual implementations of the gesture handlers.

osdnk commented 5 years ago

Also, please provide some guide how to test it. Maybe try to extend our Example app with web support and add some script for simple start?

jaulz commented 5 years ago

Sure, I will tackle the example next week. For the time being I enhanced the TapGestureHandler to respect most of the props except for minPointers. Actually the touches attribute of the event (https://developer.mozilla.org/en-US/docs/Web/API/TouchEvent/touches) should return the number of pointers but for some reason it does return an empty array both on desktop and on mobile devices and at maximum an array with a length of 1 if pressing takes longer than a few milliseconds.

osdnk commented 5 years ago

Looks so good! I think that in the future we're going to split handlers into separated files and then we could simply do e.g. TapHandler.web.js. But now, it's perfect as is! 😍

jaulz commented 5 years ago

@osdnk finally I could make the example work... you can run it via yarn run web. Since the last commit I did a major rewrite because I thought rewriting the whole GestureHandler.js would not make sense and now I just outsourced the parts that needed a different implementation on web and native. I also removed the *.web suffix so we can offer the same for any other non iOS or Android platform.

By the way, I upgraded react-navigation to ^3 so it's basically now another layer of testing within the example 😏

jaulz commented 5 years ago

I'm not 100% sure about the suffixes but e.g., the use of createRef in DrawerLayout already limits us to react@^16.3 so I assume it already limits us to newer React Native versions. Is there any mapping between React Native and React versions?

Anyway I renamed the files and we could always change it again 😊

EvanBacon commented 5 years ago

@osdnk

I mean that I am not convinced if using .native suffix for native platforms and no suffix for web will always fallback properly for old version of RN.

The .native suffix has been around since at least April 2018. @jaulz I would suggest that native code would go without a suffix and web code would have the .web suffix. This would provide maximum compatibility.

jaulz commented 5 years ago

@EvanBacon yep, it's like this now

Jekiwijaya commented 5 years ago

@jaulz the android example currently is cannot be build. Here is the fix, https://github.com/jaulz/react-native-gesture-handler/pull/1

Jekiwijaya commented 5 years ago

Much appreciate if this could go in, my projects using react-native-web and react-navigation, which this pull request could make my projects upgrade from react-navigation v2 to react-navigation v3 in web platform.

Anyway, thanks for great library! 👍 👍 👍

orestis-z commented 5 years ago

I think package.json has to be adapted to include the new files (Directions.js, State.js, ...), currently they aren't fetched by npm

jaulz commented 5 years ago

@zamponotiropita thanks for the hint

orestis-z commented 5 years ago

@jaulz Still not packaging correctly on my side, for some reason GestureHanlderModule*.js are ommited

Tested with following dependency in package.json: "react-native-gesture-handler": "jaulz/react-native-gesture-handler#fix/web-compatibility",

jaulz commented 5 years ago

@zamponotiropita can you check if the package.json is up to date? Maybe it's still cached.

orestis-z commented 5 years ago

@jaulz Tested after doing rm -r node_modules package-lock.json && npm i

Rest of the files are present, it's only regarding GestureHanlderModule*.js, did you test it with npm on your side?

jaulz commented 5 years ago

@zamponotiropita please try again now.

jaulz commented 5 years ago

@kmagiera thanks for your feedback! I just pushed some changes which should improve the overall structure. Could you please have another look?

orestis-z commented 5 years ago

@jaulz super funkt danke

kmagiera commented 5 years ago

Thanks @jaulz for responding to my comments – this looks way better now!

I might've not been clear enough with one of my suggestion earlier and would like to run this idea through you once more. So when it comes to the code split between web and mobile I was more thinking if we could have separate implementations of createHandler method (line 166). In my opinion this way we could avoid introducing unnecessary abstractions for "renderer" and "mobule" and have web version use your implementation while mobile version use native modules. We would also avoid having so many web-related check in createHandler.

I am not completely sure if that suggestion wouldn't complicate some other things, so would appreciate if you could share what you think about it before you invest your time in making the change

jaulz commented 5 years ago

@kmagiera I understand what your point is but the reason why I chose this way is because we can reuse all the things are common (e.g. prop types, resolving refs etc.) without a major rewrite of the GestureHandler. Also, going forward the web related checks might not only be applicable for web only but also for other platforms - maybe other platforms require attachGestureHandler as well? In that case the current set up will help us to have consistent GestureHandlers across multiple platforms.

kmagiera commented 5 years ago

I see. However in my opinion we don't need this whole ref hackery if handlers are mapped to the actual components. So I am not sure if there is any value in reusing these.

What are "web related checks" you mentioned? To be honest I am more concerned about someone accidentally breaking web compatibility because it may not be clear for them in what circumstances module methods are not present.

jaulz commented 5 years ago

To be honest I'm not 100% sure if the ref hacks are needed on web (e.g., are there cases when ref.current is null?) but we might need the ref hacks for other platforms and their native modules in case there is demand in the future (e.g. the ones listed here: https://facebook.github.io/react-native/docs/out-of-tree-platforms).

With "web related checks" I meant the checks that are not handled via different file name suffixes which are these at the moment: https://github.com/kmagiera/react-native-gesture-handler/pull/406/files#diff-56d925ec2aada9e1b3b613fe5b782837R206

https://github.com/kmagiera/react-native-gesture-handler/pull/406/files#diff-56d925ec2aada9e1b3b613fe5b782837R220

https://github.com/kmagiera/react-native-gesture-handler/pull/406/files#diff-56d925ec2aada9e1b3b613fe5b782837R230

https://github.com/kmagiera/react-native-gesture-handler/pull/406/files#diff-56d925ec2aada9e1b3b613fe5b782837R238

Since I do not have a preferred option and am open for both: shall I come up with another commit with createHandler.js and createHandler.web.js and incorporate the current GestureHandlerModule and GestureHandlerRenderer in those?

kmagiera commented 5 years ago

I think from a maintainer perspective it would be easier for us to accept it if we have fewer "diversion points". So if you think this wouldn't take too much time I'd be glad if you could change it the way you proposed with createHandler.js and createHandler.web.js

axhamre commented 5 years ago

May I ask what's the status here?

jaulz commented 5 years ago

@kmagiera could you please check the latest version?

iamnader commented 5 years ago

excited for this. thanks for all the work here folks

jaulz commented 5 years ago

@kmagiera do you have any timeline for a new release? Thanks for merging by the way 😊

kmagiera commented 5 years ago

will try to send out new release tomorrow

alidcast commented 5 years ago

just tried using release with this PR for web and ran into errors. what do we need to configure to get it running - looks like index.web needs to be resolved over index, anything else?

it'd be great we we could just have a simple alias (like we do react-native to react-native-web)

jaulz commented 5 years ago

@alidcastano if you add the .web.js suffix to your Webpack configuration (in resolve.extensions) it should work immediately.

orestis-z commented 4 years ago

I opened an issue for two encountered bugs here: https://github.com/kmagiera/react-native-gesture-handler/issues/632 Please have a look.

Manishalexin commented 4 years ago

When trying to import from RNGH in react native web project, it throws with this error, "Support for the experimental syntax 'exportDefaultFrom' isn't currently enabled", adding babel plugins for these doesn't help.

Minimum reproducible demo, https://codesandbox.io/s/polished-paper-tdmyp

brentvatne commented 4 years ago

@Manishalexin - you should use babel-preset-expo and the react-native-web setup that you get when you init a project with expo-cli - npm i -g expo-cli && expo init, or follow this guide to add to an existing project: https://docs.expo.io/versions/latest/guides/running-in-the-browser/

here's an example of it running in snack: https://snack.expo.io/@notbrent/jealous-coffee

Manishalexin commented 4 years ago

Thank you @brentvatne it's working with babel-preset-expo in babel config and I have to use expo start. React scripts & react-scripts-rewired still fails. So, now the storybook fails as it still uses react scripts. I used the setup here, https://github.com/expo/examples/tree/master/with-storybook but it doesn't work as I believe it's only for the expo projects. Any suggestions on how to make it work?

I am using a bare workflow, with react native cli.

jpaas commented 3 years ago

Also running into this on a create-react-app project using RNGH 1.6.1

SyntaxError: .../node_modules/react-native-gesture-handler/DrawerLayout.js: Support for the experimental syntax 'flow' isn't currently enabled (30:8):

  28 | const SETTLING = 'Settling';
  29 |
> 30 | export type PropType = {
     |        ^
  31 |   children: any,
  32 |   drawerBackgroundColor?: string,
  33 |   drawerPosition: 'left' | 'right',
afilp commented 3 years ago

I do not use expo, how can we solve the dreaded /react-native-gesture-handler/DrawerLayout.js: Support for the experimental syntax 'exportDefaultFrom' isn't currently enabled (30:8): issue? I tried using react-app-rewired but this does not seem to work, I get other errors and I am not sure if we should go this way.

Thanks a lot!

I also tried the expo solution (although the app is not an expo app), but now I get this weird error:

 web  Failed to compile.
D:/R/myApp/node_modules/react-native/Libraries/Network/XMLHttpRequest.js
Module not found: Can't resolve './RCTNetworking' in 'D:\R\myApp\node_modules\react-native\Li
braries\Network'
Error from chokidar (D:\): Error: EBUSY: resource busy or locked, lstat 'D:\pagefile.sys'

EDIT: This was related to reactotron, so not an issue, see here for solution: https://forums.expo.io/t/failed-to-compile-cant-resolve-rctnetworking/26273/7

kopax-polyconseil commented 2 years ago

Hi, I am trying to add react-native-web support on an existing react native ios android application,

I still have error such as this one : 

 FAIL  src/features/search/pages/__tests__/SearchFilter.test.tsx
  ● Test suite failed to run

    TypeError: (0 , _reactNative.requireNativeComponent) is not a function

      1 | import { t } from '@lingui/macro'
      2 | import React, { useState } from 'react'
    > 3 | import { TouchableOpacity } from 'react-native-gesture-handler'
        | ^
      4 | import styled from 'styled-components/native'
      5 | 
      6 | import { DateFilter } from 'features/search/atoms/Buttons'

      at Object.<anonymous> (node_modules/react-native-gesture-handler/GestureHandlerButton.js:3:32)
      at Object.<anonymous> (node_modules/react-native-gesture-handler/GestureButtons.js:6:1)
      at Object.<anonymous> (node_modules/react-native-gesture-handler/GestureHandler.js:10:1)
      at Object.<anonymous> (node_modules/react-native-gesture-handler/Swipeable.js:10:1)
      at Object.<anonymous> (node_modules/react-native-gesture-handler/index.js:1:1)
      at Object.<anonymous> (src/features/search/sections/OfferDate.tsx:3:1)
      at Object.<anonymous> (src/features/search/sections/index.ts:8:1)
      at Object.<anonymous> (src/features/search/pages/SearchFilter.tsx:10:1)
      at Object.<anonymous> (src/features/search/pages/__tests__/SearchFilter.test.tsx:12:1)

Looking at your discussion, I assume it should work out of the box with react-native-gesture-handler v1.6.0, why am I still getting those requireNative? Thanks a lot for this awsome lib!