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

Add PureComponent fallback babel transform #1452

Merged
merged 1 commit into from
Nov 6, 2018

Conversation

majapw
Copy link
Collaborator

@majapw majapw commented Nov 6, 2018

Fix for #1354

Never got around to doing this before and now it is blocking.

Thanks @skevy for writing the transform!

to: @ljharb @skevy @Sumeet-Jain

@majapw majapw added the semver-patch: fixes/refactors/etc Anything that's not major or minor. label Nov 6, 2018
@coveralls
Copy link

coveralls commented Nov 6, 2018

Coverage Status

Coverage increased (+0.04%) to 85.049% when pulling 443c600 on maja-add-purecomponent-babel-transform into 89f8c37 on master.

@majapw majapw force-pushed the maja-add-purecomponent-babel-transform branch from 7b5c78f to e9b8303 Compare November 6, 2018 01:52
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This needs to also set the pureComponent RWS option to false, when there's no PureComponent

.babelrc Outdated
@@ -7,7 +7,8 @@
"svgo": false
}],
["transform-replace-object-assign", { "moduleSpecifier": "object.assign" }],
"istanbul"
"istanbul",
Copy link
Member

Choose a reason for hiding this comment

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

i think the istanbul transform needs to come last, so it can properly instrument the code for coverage

@majapw majapw force-pushed the maja-add-purecomponent-babel-transform branch 4 times, most recently from aec0c32 to 47ba050 Compare November 6, 2018 02:34
@majapw
Copy link
Collaborator Author

majapw commented Nov 6, 2018

@ljharb @skevy i think this makes sense now!

@@ -0,0 +1,67 @@
module.exports = function pureComponentFallback(babel) {
const { types: t, template } = babel;
Copy link
Member

Choose a reason for hiding this comment

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

nbd but this can be destructured in the previous line

const superclass = path.get('superClass');
// check for PureComponent
if (t.isIdentifier(superclass)) {
return superclass.node.name === 'PureComponent';
Copy link
Member

Choose a reason for hiding this comment

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

probably fine for "just this repo", but if we'll want to use this in other projects, we should make sure that PureComponent is actually a named import from 'react' or a destructure from React imported/required from 'react'


if (t.isMemberExpression(superclass)) {
// Check for React.PureComponent
return superclass.get('object').node.name === 'React'
Copy link
Member

Choose a reason for hiding this comment

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

same here

@@ -340,4 +338,4 @@ export default withStyles(({ reactDates: { color, font } }) => ({
CalendarDay__today: {},
CalendarDay__firstDayOfWeek: {},
CalendarDay__lastDayOfWeek: {},
}), { pureComponent: pureComponentAvailable })(CalendarDay);
}), { pureComponent: typeof React.PureComponent !== 'undefined' })(CalendarDay);
Copy link
Member

Choose a reason for hiding this comment

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

not a blocker, but it'd be nicer if the transform could add this instead

'method',
t.identifier('shouldComponentUpdate'),
[t.identifier('nextProps'), t.identifier('nextState')],
t.blockStatement([template('return shallowCompare(this, nextProps, nextState);')()]),
Copy link
Member

@ljharb ljharb Nov 6, 2018

Choose a reason for hiding this comment

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

the trailing function comma here is breaking node 6.

@majapw
Copy link
Collaborator Author

majapw commented Nov 6, 2018

I tested this in the react-dates-demo repo (https://www.github.com/majapw/react-dates-demo) with React 15.2 and React 16 in both Chrome and IE10 successfully.

@majapw majapw force-pushed the maja-add-purecomponent-babel-transform branch from 47ba050 to 04e5ba9 Compare November 6, 2018 19:26
@majapw majapw force-pushed the maja-add-purecomponent-babel-transform branch from 04e5ba9 to 443c600 Compare November 6, 2018 21:13
@majapw majapw merged commit a04dfd1 into master Nov 6, 2018
@majapw majapw deleted the maja-add-purecomponent-babel-transform branch November 6, 2018 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch: fixes/refactors/etc Anything that's not major or minor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants