Remove newly unreferenced VariableDeclarators #146

lencioni · 2018-07-25T19:03:06Z

I've noticed that a common pattern used to assign propTypes--first
define a variable for the propTypes, and then assign it to the component
by reference--ends up deoptimizing this plugin, which leads to larger
bundles in production than desired. The original hypothesis that
minification would clean up these dead references works well for the
simplest of propTypes, but unfortunately it doesn't hold up as
soon as you have a function call in the propTypes variable (such as is
the case for propTypes like arrayOf, objectOf, shape, etc.)
because the minfier does not know that the function call will not have
side-effects:

const propTypes = {
  foo: PropTypes.arrayOf(PropTypes.number),
};

I believe this should be solved by this plugin because we can make the
assumption that function calls inside of propTypes have no side-effects.

There seems to be a weird issue where the transform adds a semicolon
when using in wrapped mode. For now, I decided to simply add the
additional semicolons in the test fixtures, since they will be minified
out anyway.

I don't have much experience working with Babel plugins yet, so it is
likely that I'm doing things in a weird or sub-optimal way here.

Fixes #145

oliviertassinari · 2018-07-25T19:19:10Z

@lencioni Thanks for the failing test case. It comes down to this plugin only removing the reference between the React component and the prop types, deferring potential dead code elimination to another tool.

oliviertassinari · 2018-07-25T19:20:46Z

I guess #145 and #144 are two flavors of the same core issue. I'm not sure I will prioritize this effort in the near future. I have been pouring most of my free time in OSS to Material-UI.

lencioni · 2018-07-25T19:36:32Z

Yeah, I think that's a pretty accurate assessment. If I put up a PR that addresses this, would you be open to getting it merged in and published?

oliviertassinari · 2018-07-25T19:41:19Z

@lencioni It might increase the time this plugin takes, but nobody is benchmarking that, so we are good. As for the potential bug it might introduce, I have trust in you, so yes, I'm more than happy to review such work!

lencioni · 2018-07-25T20:06:53Z

@oliviertassinari Sounds great! Before I begin working on an implementation, do you think the test cases I've added here are adequate? Or are there more cases we want to test against?

oliviertassinari · 2018-07-25T20:12:27Z

Or are there more cases we want to test against?

Maybe one more with an export of the proptypes? I can't think of anything else.

lencioni · 2018-07-31T16:37:37Z

@oliviertassinari I've updated this PR with a solution that passes all tests. I've never really worked with Babel plugins before, so it is likely that I've made some weird decisions. I'd love your feedback on this!

lencioni · 2018-07-31T19:44:10Z

As a sanity check, I ran this on our large internal component library project and it seems to be doing the right thing.

raejin · 2018-07-31T19:45:50Z

src/index.js

+
+      if (
+        path.parent.type === 'ObjectProperty' &&
+        (path.parent.key === path.node || path.parent.shorthand)


Do we need to check if path.parent.computed?

{ [computedKey]: 'foo' }

No, I think we still want identifiers in computed object keys, since by removing the object we may no longer need the identifier used in the computed key and we will want to clean those up.

But I think we might not actually need the next check for CallExpressions. I'll remove that now.

lencioni · 2018-07-31T21:12:23Z

src/index.js

+        return
+      }
+
+      if (path.parent.type === 'CallExpression' && path.parent.callee === path.node) {


I think we don't actually need this check. I don't actually remember why I added it in the first place.

I've noticed that a common pattern used to assign propTypes--first define a variable for the propTypes, and then assign it to the component by reference--ends up deoptimizing this plugin, which leads to larger bundles in production than desired. The original hypothesis that minification would clean up these dead references works well for the simplest of propTypes, but unfortunately it doesn't hold up as soon as you have a function call in the propTypes variable (such as is the case for propTypes like `arrayOf`, `objectOf`, `shape`, etc.) because the minfier does not know that the function call will not have side-effects: ```js const propTypes = { foo: PropTypes.arrayOf(PropTypes.number), }; ``` I believe this should be solved by this plugin because we can make the assumption that function calls inside of propTypes have no side-effects. There seems to be a weird issue where the transform adds a semicolon when using in wrapped mode. For now, I decided to simply add the additional semicolons in the test fixtures, since they will be minified out anyway. I don't have much experience working with Babel plugins yet, so it is likely that I'm doing things in a weird or sub-optimal way here.

lencioni · 2018-08-04T05:44:43Z

@oliviertassinari are there any changes you want me to make to this PR?

oliviertassinari · 2018-08-06T18:34:54Z

@lencioni Thank you for your time! No, I need to find some to review the pull request :)

adamrneary · 2018-08-16T18:21:39Z

Great PR or greatest PR? This is fabulous. +1000

ljharb

Are there any tests to cover React.createClass/create-react-class?

ljharb · 2018-08-16T18:53:00Z

src/index.js

@@ -53,9 +53,39 @@ function isReactClass(superClass, scope, globalOptions) {
  return answer
 }

+function areSetsEqual(set1, set2) {
+  if (set1.size !== set2.size) {


even tho it doesn't matter for this use case, i'd probably add an additional bailout; if (set1 === set2) { return true; } :-)

ljharb · 2018-08-16T18:55:50Z

test/fixtures/es-class-assign-property-variable/expected-wrap-es5.js

@@ -1,8 +1,8 @@
 "use strict";

-var propTypes = {
+var propTypes = process.env.NODE_ENV !== "production" ? {
  foo: PropTypes.string


Can you add some tests for get bar() { return PropTypes.string; } and similar?

Also, it'd be good to ensure that this doesn't crash:

var propTypes = { ...({ foo: PropTypes.string }), };

ljharb · 2018-08-16T18:56:30Z

test/fixtures/es-class-assign-property-variable/expected-wrap-es5.js

  foo: PropTypes.string
-};
+} : {};;


are the double semicolons intended?

There seems to be a weird issue where the transform adds a semicolon
when using in wrapped mode. For now, I decided to simply add the
additional semicolons in the test fixtures, since they will be minified
out anyway.

If you know why they are there, I'd love to fix!

oh oops, missed that.

i don't know why; not that big a deal ofc.

ljharb · 2018-08-16T18:56:36Z

test/fixtures/es-class-assign-property-variable/expected-wrap-es6.js

  foo: PropTypes.string
-};
+} : {};;


ljharb · 2018-08-16T18:56:58Z

test/fixtures/variable-assignment/actual.js

+  bar: PropTypes.string
+};
+
+const propTypesWithExtraReference = Object.assign({}, extraReference, {


an object spread example could be good alongside this

Although this probably doesn't matter for our use-case, it is more complete.

This gives us more assurance that this code is working as expected.

I'm not entirely sure if this is the desired result, but I think it maybe is, since anything could be using the getter.

lencioni · 2018-08-16T23:32:01Z

@ljharb Thanks for the review! I've added some tests.

lencioni · 2018-08-22T16:18:12Z

@oliviertassinari If you are too busy to work on this, I'd be happy to merge this and publish a new version of the plugin for you if you want.

oliviertassinari · 2018-08-22T16:55:45Z

@lencioni I would love that. Let me send the needed invitation. Thanks.

lencioni · 2018-08-22T17:06:48Z

My npm username is lencioni

lencioni · 2018-08-22T19:38:54Z

@oliviertassinari thanks for the permissions on this repo! I've merged this change and once you give me publish access on npm, I can get a new release out.

https://www.npmjs.com/package/babel-plugin-transform-react-remove-prop-types

oliviertassinari · 2018-08-22T19:45:42Z

@lencioni You should already have the access.

ljharb · 2018-08-22T20:17:00Z

The website pages are cached and take awhile to update; try npm owner ls babel-plugin-transform-react-remove-prop-types to verify.

lencioni · 2018-08-22T21:00:24Z

The npm owner command seems to be cached as well. Regardless, I was able to publish!

oliviertassinari · 2018-08-22T21:22:01Z

I can't wait to try it out 🎉 !

ljharb

thought of this later

ljharb · 2018-08-23T18:15:36Z

src/index.js

 export default function(api) {
  const { template, types, traverse } = api

+  const nestedIdentifiers = new Set()
+  const removedPaths = new Set()


this contains objects, and is never traversed, so it probably should be a WeakSet

lencioni mentioned this pull request Jul 25, 2018

Feature request: remove unused propTypes intermediate variable #145

Closed

lencioni force-pushed the variable-assignment branch from 7c9e2f0 to d048465 Compare July 25, 2018 19:12

lencioni force-pushed the variable-assignment branch 2 times, most recently from db98c38 to 990c22a Compare July 31, 2018 16:36

lencioni changed the title ~~Add failing tests for propTypes from variable assignment~~ Remove newly unreferenced VariableDeclarators Jul 31, 2018

lencioni force-pushed the variable-assignment branch from 990c22a to 9001851 Compare July 31, 2018 16:53

raejin reviewed Jul 31, 2018

View reviewed changes

lencioni commented Jul 31, 2018

View reviewed changes

lencioni force-pushed the variable-assignment branch from 9001851 to 07b667c Compare July 31, 2018 21:12

oliviertassinari self-assigned this Aug 13, 2018

ljharb suggested changes Aug 16, 2018

View reviewed changes

lencioni added 4 commits August 16, 2018 13:56

Add early bailout in areSetsEqual()

56a7de5

Although this probably doesn't matter for our use-case, it is more complete.

Add test to variable-assignment case for object spread

8ee80d1

This gives us more assurance that this code is working as expected.

Add a getter test to es-class-assign-property-variable

c8eb10d

I'm not entirely sure if this is the desired result, but I think it maybe is, since anything could be using the getter.

Add variable-assignment tests for create-react-class

f4b25a3

ljharb approved these changes Aug 17, 2018

View reviewed changes

lencioni merged commit 4575801 into oliviertassinari:master Aug 22, 2018

lencioni deleted the variable-assignment branch August 22, 2018 18:59

ljharb reviewed Aug 23, 2018

View reviewed changes

lencioni mentioned this pull request Feb 1, 2019

Fix removing declarator type for unsafe-wrap mode. Fixes #175 #180

Merged

Remove newly unreferenced VariableDeclarators #146

Remove newly unreferenced VariableDeclarators #146

Conversation

lencioni commented Jul 25, 2018 • edited Loading

oliviertassinari commented Jul 25, 2018

oliviertassinari commented Jul 25, 2018 • edited Loading

lencioni commented Jul 25, 2018

oliviertassinari commented Jul 25, 2018

lencioni commented Jul 25, 2018

oliviertassinari commented Jul 25, 2018

lencioni commented Jul 31, 2018

lencioni commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lencioni commented Aug 4, 2018

oliviertassinari commented Aug 6, 2018

adamrneary commented Aug 16, 2018

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lencioni commented Aug 16, 2018

lencioni commented Aug 22, 2018

oliviertassinari commented Aug 22, 2018

lencioni commented Aug 22, 2018

lencioni commented Aug 22, 2018 • edited Loading

oliviertassinari commented Aug 22, 2018

ljharb commented Aug 22, 2018

lencioni commented Aug 22, 2018

oliviertassinari commented Aug 22, 2018 • edited Loading

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lencioni commented Jul 25, 2018 •

edited

Loading

oliviertassinari commented Jul 25, 2018 •

edited

Loading

lencioni commented Aug 22, 2018 •

edited

Loading

oliviertassinari commented Aug 22, 2018 •

edited

Loading