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

Update hammerjs to use React.Component and react prop-types #78

Merged
merged 5 commits into from
Oct 17, 2017

Conversation

chrisdoc
Copy link
Contributor

This PR makes HammerJS compatibly with React 16 which removed React.createClass.

In order to make npm run build work I updated several dev dependencies and included babelify to transpile the ES6 class

@halkeye
Copy link

halkeye commented Sep 29, 2017

the other option is the polyfill - https://www.npmjs.com/package/create-react-class

i like your version better

Copy link

@oayres oayres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chrisdoc could you change module.exports = to export default? This does matter for certain compilers some of us may use in projects. Also, babel-preset-env may be favoured over ES2015 and stage-0.

Copy link

@oayres oayres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Chris, looks good - @JedWatson please can we get this merged in? This change is required for any new React project.

@lostthetrail
Copy link

lostthetrail commented Oct 9, 2017

The package.json main attribute is no longer correct for node integrations, as node doesn't support the import/export syntax.

It seems odd to mix syntax between the require at the top and the export at the bottom.

@lostthetrail
Copy link

@chrisdoc Thanks for putting this PR in. Did you see my note about the package.json main attribute no longer being valid. This is interesting to me since I am doing some SSR and the file breaks NodeJS parsing due to the unexpected module syntax.

@chrisdoc
Copy link
Contributor Author

@lostthetrail thank you for the feedback, I change it to use import and export. Can you also tell me how you are using it in combination with node? We do use SSR too but haven't tried it out with the updated version.

@lostthetrail
Copy link

@chrisdoc We server side render our react. In doing so, the whole module tree is loaded in Node. For Node to do this, it reads the required package.json main attribute to find more Node-compatible javascript to load. Currently, with your changes, the main attribute points to code that is not Node-compatible, due to, at least, the usage of import and export.

@halkeye
Copy link

halkeye commented Oct 16, 2017

As a work around you could use https://github.com/standard-things/esm/blob/master/README.md

@chrisdoc
Copy link
Contributor Author

@lostthetrail you could use babel in the backend, how are transpiling JSX in node?

@JedWatson JedWatson merged commit 45d31c4 into JedWatson:master Oct 17, 2017
@JedWatson
Copy link
Owner

Thanks @chrisdoc and everyone for the review to get this ready for merge.

Sorry for the delay - I know this must have been frustrating, so thanks to the helpful twitter user who brought it to my attention.

I'll try and sort out the node version without breaking our ability to use import/export but otherwise it might be better to (in the short term) revert to require and module.exports for node compatibility then add es modules in a compatible way with a subsequent release.

Anyone have any strong preferences on that?

@oayres
Copy link

oayres commented Oct 17, 2017

@JedWatson thinking for the many and not the few here: If somebody is using JSX on a Node server without transpilation, maybe we can just include a transpiled version in the npm module? More recent babel versions should assign the export default to module.exports.

So those that do not support import/export can just import it differently:

const Hammer = require('react-hammerjs/lib/es5')

Either way, getting it merged as-is will suffice!

@rhys-vdw
Copy link

@JedWatson Can we get a release please?

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

Successfully merging this pull request may close these issues.

6 participants