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

Added support for ES6 imports and exports (and PureComponent) #26

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vdh
Copy link

@vdh vdh commented Dec 12, 2016

Fixes #23 & #8 by checking imports for import React, { Component } from 'react', and also adds support for the new PureComponent.

It possibly also fixes #22 due to the fix for export declarations (which are mostly only used in the tests, since Babel already converts the ES6 exports down to ES5 syntax).

  • Updated babel-helper-is-react-class
    • Version bump to 2.0.0 because of the API change (requires path instead of path.node)
    • Added a traversal for named imports (e.g. import React, { Component } from 'react')
    • Added checks for PureComponent
  • Updated babel-plugin-transform-react-pure-class-to-function
    • Refactored replacement logic to read more clearly now that there's 3 code paths
    • Added export declaration check, to avoid "We don't know what to do with this node type" errors
    • Added extra tests for new functionality

- Updated `babel-helper-is-react-class`
    - Version bump to `2.0.0` because of the API change (requires `path` instead of `path.node`)
    - Added a traversal for named imports (e.g. `import React, { Component } from 'react'`)
    - Added checks for `PureComponent`
- Updated `babel-plugin-transform-react-pure-class-to-function`
    - Refactored replacement logic to read more clearly now that there's 3 code paths
    - Added export declaration check, to avoid "We don't know what to do with this node type" errors
    - Added extra tests for new functionality
vdh pushed a commit to vdh/babel-react-optimize that referenced this pull request Dec 12, 2016
@vdh
Copy link
Author

vdh commented Mar 24, 2017

@LeoIannacone Well, I've made a temporary fork on NPM that I've been using: babel-plugin-transform-react-pure-class-to-function-fork

I was planning on deprecating it once this PR got merged.

@@ -0,0 +1 @@
node_modules
Copy link
Collaborator

Choose a reason for hiding this comment

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

From the eslint docs:

In addition to any patterns in a .eslintignore file, ESLint always ignores files in /node_modules/* and /bower_components/*.

@vdh
Copy link
Author

vdh commented Aug 17, 2017

@thedillonb Wouldn't a shouldComponentUpdate inherently contain references to this, and thus would skip the transformation?

@vdh
Copy link
Author

vdh commented Aug 18, 2017

@thedillonb Actually, after a cursory glance over the plugin, it looks like anything that contains more than just render, static propTypes, or static defaultProps will not be transformed.

@thedillonb
Copy link

thedillonb commented Aug 18, 2017

@vdh, you're right. If there are methods other than render then it will not be transformed. However, in your test case for test/fixtures/pure-component/actual.js and test/fixtures/pure-component/expected.js, the expected is incorrect. Converting a PureComponent into a stateless functional component (SFC) drops the functionality. React.PureComponent implements componentShouldUpdate for you as a shallowCompare a SFC does not do any comparisons on the prop types and with thus always re-render even if the props are the same (see facebook/react#5677, and more specifically facebook/react#5677 (comment) and facebook/react#5677 (comment)).

@vdh
Copy link
Author

vdh commented Aug 22, 2017

@thedillonb Hmm, that's sort of a limitation in how React currently implements the rendering of stateless functional components, but I get what you're saying.

Perhaps if I added some sort of optional configuration toggle to skip PureComponent? Something like ignorePureComponent to mirror the related config in the prefer-stateless-function rule from eslint-plugin-react.

@basilfx
Copy link

basilfx commented Oct 3, 2017

I have forked and revived the original plugin (here, don't like the idea of having presets and plugins mixed) and made it up to date with some of the pull requests in here.

I have added the option (disabled by default) to convert PureComponents or not, and added some additional features.

Repository owner deleted a comment from thedillonb Oct 3, 2017
Repository owner deleted a comment from LeoIannacone Oct 3, 2017
Repository owner deleted a comment from LeoIannacone Oct 3, 2017
Repository owner deleted a comment from mehcode Oct 3, 2017
@jamiebuilds
Copy link
Owner

You know, maybe if a single person in any of these PRs offered to help out maintaining this project, then it would be more active. But everyone just wants to complain. So you all get nothing

@havenchyk
Copy link
Collaborator

@thejameskyle I can try at least to go though the opened issues. I use the plugin in production, so I'm interested in it. Would you mind to add some permissions to me?

@jamiebuilds
Copy link
Owner

Added

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