Skip to content
This repository has been archived by the owner on Sep 11, 2018. It is now read-only.

[Babel] Improve the production build #561

Merged
merged 1 commit into from
Feb 18, 2016
Merged

[Babel] Improve the production build #561

merged 1 commit into from
Feb 18, 2016

Conversation

oliviertassinari
Copy link
Contributor

Add some plugins for improving the production build.

Name Build size impact Description
transform-react-constant-elements 0 kB Save some memory allocation.
transform-react-inline-elements +0.7 kB Improve runtime performances.
transform-react-remove-prop-types (Promoted) -0.3 kB Remove propTypes.

@codecov-io
Copy link

Current coverage is 47.72%

Merging #561 into master will not affect coverage as of 5b04069

@@            master    #561   diff @@
======================================
  Files            6       6       
  Stmts           44      44       
  Branches         0       0       
  Methods          0       0       
======================================
  Hit             21      21       
  Partial          0       0       
  Missed          23      23       

Review entire Coverage Diff as of 5b04069

Powered by Codecov. Updated on successful CI builds.

@dvdzkwsk
Copy link
Owner

Woah, this is awesome, thank you! Will check it out when I get home.

@dvdzkwsk
Copy link
Owner

@oliviertassinari the remove-prop-types plugin works perfectly 👍 . However, I tried adding these to a bigger application forked from this starter kit, just to see how it performs in a bigger code base, and I had a smattering of these build errors (caused by one of the other two plugins), so I'm a bit wary of just dropping them in without further testing:

./src/components/<Component>.js
Module build failed: TypeError: /Users/zuko/projects/<my-project>/src/components/<Component>.js: Property object of JSXMemberExpression expected node to be of a type ["JSXMemberExpression","JSXIdentifier"] but instead got "MemberExpression"
    at Object.validate (/Users/zuko/projects/<my-project>/node_modules/babel-types/lib/definitions/index.js:101:13)
    at Object.validate (/Users/zuko/projects/<my-project>/node_modules/babel-types/lib/index.js:269:9)
    at NodePath._replaceWith (/Users/zuko/projects/<my-project>/node_modules/babel-traverse/lib/path/replacement.js:201:7)
    at NodePath.replaceWith (/Users/zuko/projects/<my-project>/node_modules/babel-traverse/lib/path/replacement.js:179:8)
    at Object.ReferencedIdentifier (/Users/zuko/projects/<my-project>/node_modules/babel-plugin-transform-es2015-modules-commonjs/lib/index.js:56:14)
    at Object.newFn (/Users/zuko/projects/<my-project>/node_modules/babel-traverse/lib/visitors.js:326:17)
    at NodePath._call (/Users/zuko/projects/<my-project>/node_modules/babel-traverse/lib/path/context.js:74:18)
    at NodePath.call (/Users/zuko/projects/<my-project>/node_modules/babel-traverse/lib/path/context.js:46:17)
    at NodePath.visit (/Users/zuko/projects/<my-project>/node_modules/babel-traverse/lib/path/context.js:104:12)
    at TraversalContext.visitQueue (/Users/zuko/projects/<my-project>/node_modules/babel-traverse/lib/context.js:156:16)
    at TraversalContext.visitSingle (/Users/zuko/projects/<my-project>/node_modules/babel-traverse/lib/context.js:113:19)
    at TraversalContext.visit (/Users/zuko/projects/<my-project>/node_modules/babel-traverse/lib/context.js:200:19)
    at Function.traverse.node (/Users/zuko/projects/<my-project>/node_modules/babel-traverse/lib/index.js:139:17)
    at NodePath.visit (/Users/zuko/projects/<my-project>/node_modules/babel-traverse/lib/path/context.js:114:22)
    at TraversalContext.visitQueue (/Users/zuko/projects/<my-project>/node_modules/babel-traverse/lib/context.js:156:16)
    at TraversalContext.visitMultiple (/Users/zuko/projects/<my-project>/node_modules/babel-traverse/lib/context.js:108:17)
    at TraversalContext.visit (/Users/zuko/projects/<my-project>/node_modules/babel-traverse/lib/context.js:198:19)
    at Function.traverse.node (/Users/zuko/projects/<my-project>/node_modules/babel-traverse/lib/index.js:139:17)
 @ ./src/containers/<AnotherComponent>.js 50:18-61',

Have you seen this or have any idea as to what the issue might be? I know it's hard to tell without source code, but I'd have to put together a minimal example since this is from a private codebase. This issue doesn't occur if I only include the remove-prop-types plugin; haven't tested the other two in isolation yet.

@oliviertassinari
Copy link
Contributor Author

This issue doesn't occur if I only include the remove-prop-types plugin; haven't tested the other two in isolation yet.

That's an interesting issue! Indeed, that's hard to tell like this.
Could you try with one plugin at the time?

@dvdzkwsk
Copy link
Owner

  • PASS: remove-prop-types
  • PASS: remove-prop-types and constant-elements
  • FAIL: react-inline-elements (error posted above)
  • FAIL: remove-prop-types and react-inline-elements (error posted above)

So it looks like it's just react-inline-elements. Will try to take a look at the files it's erroring on later.

@dvdzkwsk
Copy link
Owner

@oliviertassinari if you want to resubmit without react-inline-elements we can get this merged... react-inline-elements would be nice as well, but I'd rather avoid a (potential) onslaught of issues caused by the plugin if it has an edge case or two.

@oliviertassinari
Copy link
Contributor Author

  • Removed babel-plugin-transform-react-inline-elements.

I'm curious to know what is causing this issue.
We are using this babel-plugin-transform-react-inline-elements plugin here http://www.material-ui.com. I'm also using it here https://splitme.net. That's working fine so far.

@dvdzkwsk
Copy link
Owner

@oliviertassinari I will try to get a minimal example to repro, but just in the goal of safety until I know whether it's a problem with my code (likely) or the transform I'd rather err on the side of caution. Thanks for the update!

@dvdzkwsk dvdzkwsk merged commit c953454 into dvdzkwsk:master Feb 18, 2016
@dvdzkwsk
Copy link
Owner

Merged, thank you. If you'd like to open up another PR for the other transform we can continue the discussion there, or I can just open one up myself later.

@oliviertassinari
Copy link
Contributor Author

@davezuko Thanks! Yeah, we can continue here.

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

Successfully merging this pull request may close these issues.

3 participants