JedWatson / react-hammerjs

ReactJS / HammerJS integration. Support touch events in your React app.
MIT License
937 stars 129 forks source link

Release React 16 changes #81

Closed Redmega closed 7 years ago

Redmega commented 7 years ago

I am a collaborator on https://www.github.com/stoeffel/react-motion-drawer and we depend on this project. You recently merged the update to allow React 16 to work, but there hasn't been a new release yet! Is there any way you can create a new tag and publish to npm? What other changes were you planning for the next release?

rhys-vdw commented 7 years ago

I just tried depending on this commit directly and it seems that the main export is not transpiled, causing the following error:

ERROR in ./node_modules/react-hammerjs/src/Hammer.js
Module parse failed: /Users/rhys/Projects/usability-hub/usability_hub/node_modules/react-hammerjs/src/Hammer.js Unexpected token (102:23)
You may need an appropriate loader to handle this file type.
| 
| export default class HammerComponent extends React.Component {
|     static displayName = 'Hammer';
| 
|     static propTypes = {
 @ ./app/assets/javascripts/components/public-usability-tests/preference-carousel.js 13:0-36
 @ ./app/assets/javascripts/components/public-usability-tests/preference-step.js
 @ ./app/assets/javascripts/components/public-usability-tests/form-preference-step.js
 @ ./app/assets/javascripts/components/exports.js
 @ ./app/assets/javascripts/initialize-react.js
 @ ./app/assets/javascripts/site.js
 @ multi webpack-hot-middleware/client?name=js&path=http%3A%2F%2Flocalhost%3A3808%2F__webpack_hmr%2F&timeout=20000&reload=true&quiet= react-hot-loader/patch ./app/assets/javascripts/site.js
rhys-vdw commented 7 years ago

Okay, just if anyone cares...

I added the following to my webpack.config.js

    module: {
      rules: [
        // Explicitly transpile react-hammerjs because the versoin we needed was
        // not released and therefore did not have a compiled dist.
        //
        // This should be removed as soon as possible.
        //
        // https://github.com/JedWatson/react-hammerjs/issues/81#issuecomment-337798819
        {
          test: /\.jsx?$/,
          include: [path.join(__dirname, '..', 'node_modules', 'react-hammerjs', 'src')],
          loader: 'babel-loader',
          options: require('./babelrc.js'),
        },

This made everything work.

However, I then hit a problem where this.domElement was undefined here.

Since we're upgrading the React 16 today we're just going to drop react-hammerjs for now and bring it back when the release is ready. I don't have time to investigate why this is happening but I thought I'd share in case this is actually a problem in master.

EDIT: This was with React 15.6.1

Redmega commented 7 years ago

@rhys-vdw Interesting workaround but very fragile. Not worth the effort imo. Better to wait for @JedWatson (Or anyone else with publish access) to publish to npm. Thanks for the investigation though! 👍

rhys-vdw commented 7 years ago

Interesting workaround but very fragile.

It was meant to be a stopgap, but we just dropped gesture support instead for now.

josef-diazlopez commented 7 years ago

+1

You recently merged the update to allow React 16 to work, but there hasn't been a new release yet! Is there any way you can create a new tag and publish to npm? What other changes were you planning for the next release?

quantizor commented 7 years ago

I'm very close to just forking this library and releasing under a different name based on how utterly unresponsive @JedWatson is. Upvote this comment if you approve and I'll followup with the new repo.

Redmega commented 7 years ago

Was giving him the benefit of the doubt, everyone gets busy. I think someone with a PR that has been merged being made a collaborator and given npm rights would be just as good! @JedWatson thoughts? In either case we do need action on this so if I don't hear anything by Wednesday I'd say go ahead with the fork.

quantizor commented 7 years ago

@Redmega I'd normally agree with you, but PRs to do this work have been open since at least April. If @JedWatson doesn't have time for this anymore that's totally fine, but he should appoint at least one maintainer to carry the project instead of leaving it to languish.

JedWatson commented 7 years ago

I've just published 1.0.0 with the updates.

I have been really busy (juggling work, several oss projects and a very young daughter) but more importantly this took more time to review than I was expecting.

The changes introduced a real problem where the build also bundled prop-types in a way that it would always be bundled by consumers, blowing out package size. It's been a while since I worked with this particular build process so it took a bit of time to realise what was wrong with the bundle size (I did this by reading my way through the transpired bundle) and work out how to fix it, but I was also concerned about releasing something broken before I knew what was going on.

There are other issues with the build so I originally tried to port the new build system from react-select (that would be properly compatible with all the model targets including es modules, node and the browser) but ran out of time. Will get back to that once it's bundled up.

Sorry this was interpreted as being unresponsive, but if the changes I merged hadn't thrown red flags I would have published much earlier instead of taking the time to look into it.

JedWatson commented 7 years ago

After looking at it more, turns out there were issues with the default entry point (which, as @rhys-vdw pointed out, shouldn't be transpiled by webpack but there are so many possible build systems and configurations that ymmv)

So I ported the react-select build process properly (using rollup and babel-cli) and we now have:

... which are linked to the correct entries in package.json now.

There were also some indentation and formatting issues in src that had crept in with PRs so I adopted prettier for formatting and added eslint for good measure.

It should all be sorted now, for everyone, with no need for weird build artefacts and the build system has been future proofed a bit... please open a new issue if you have any problems consuming it and I'll do my best to respond.

Published as 1.0.1 because 1.0.0 would have been broken for several consumers.

quantizor commented 7 years ago

@JedWatson thank you! On Tue, Oct 24, 2017 at 9:35 AM Jed Watson notifications@github.com wrote:

After looking at it more, turns out there were issues with the default entry point (which, as @rhys-vdw https://github.com/rhys-vdw pointed out, shouldn't be transpiled by webpack but there are so many possible build systems and configurations that ymmv)

So I ported the react-select build process properly (using rollup and babel-cli) and we now have:

  • a lib build for node, thanks babel
  • a dist build for webpack / rollup / browserify and browsers with es, umd and umd.min variants

... which are linked to the correct entries in package.json now.

There were also some indentation and formatting issues in src that had crept in with PRs so I adopted prettier for formatting and added eslint for good measure.

It should all be sorted now, for everyone, with no need for weird build artefacts and the build system has been future proofed a bit... please open a new issue if you have any problems consuming it and I'll do my best to respond.

Published as 1.0.1 because 1.0.0 would have been broken for several consumers.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/JedWatson/react-hammerjs/issues/81#issuecomment-338991992, or mute the thread https://github.com/notifications/unsubscribe-auth/AAiy1g_v4mEvkgCcjEr3ZJ6_lA7_5DkNks5sveekgaJpZM4P8zBj .

Redmega commented 7 years ago

@JedWatson Thanks! Figured something was either pending or missing... I understand things get busy and you had concerns with the code, but a quick "This can't go in yet because xyz has to be addressed" would've eased peoples worries that you went AWOL or that the package was abandoned. That being said, thanks for your work on all the issues that you found! Upgrading now, I'll open an issue if I find any problems. 🥇

yuvke commented 6 years ago

Hi @JedWatson - I'm trying to import react-hammerjs in a project and when trying to pack the project with webpack, I'm getting an error for missing 'prop-types'. Am I suppose to install prop-types as part of the project?