Signup: Migrate imports and exports from CommonJS to ES2015 #11719
Conversation
jsnmoon
added
[Status] In Progress
[Type] Enhancement
jsnmoon
added
[Status] Needs Review
There was a problem hiding this comment.
The changes here look good to me. I had a comment but it isn't something that needs to change. I tested with the main flow (/start) logged in and logged out and the behavior was unchanged.
I'm AFK tomorrow so I will merge this on Monday if no one beats me to it.
| var React = require( 'react' ), | ||
| debug = require( 'debug' )( 'calypso:signup:wpcom-login' ); | ||
| import React from 'react'; | ||
| const debug = require( 'debug' )( 'calypso:signup:wpcom-login' ); |
There was a problem hiding this comment.
I think we usually use a different syntax for this:
import debugFactory from 'debug'; // sometimes named debugModule
const debug = debugFactory( 'calypso:whatever' );Per:
wp-calypso (master) ☯ spot client/ const debug | grep debug | grep require | wc -l
15
wp-calypso (master) ☯ spot client/ const debug | grep debug | grep Module | wc -l
41
wp-calypso (master) ☯ spot client/ const debug | grep debug | grep Factory | wc -l
68
I don't see a reason to not use the shorter syntax you have here, but I thought it was worth mentioning.
There was a problem hiding this comment.
👍 that's good to know. (Since this is part of a bulk change, I was wary of introducing an intermediary variable in case of collisions.)
I think using the shorter syntax has two potential downsides - you've already mentioned the first (it deviates from an established pattern in the codebase). The second is that Webpack 2 won't be able to tree shake this import due to the CommonJS notation. However, I highly doubt we'd reach a situation where we'd need to shake debug off during a build.
drewblaisdell
added
[Status] Ready to Merge
and removed
[Status] Needs Review
| assign = require( 'lodash/assign' ); | ||
| import i18n from 'i18n-calypso'; | ||
| import React from 'react'; | ||
| import assign from 'lodash/assign'; |
There was a problem hiding this comment.
We use a babel plugin that allows tree-shaking of lodash utils already, so this could be converted to:
import { assign } from 'lodash';FFTI if you feel this is out of this PR's scope though :)
| map = require( 'lodash/map' ), | ||
| debug = require( 'debug' )( 'calypso:steps:site' ); // eslint-disable-line no-unused-vars | ||
| import React from 'react'; | ||
| import isEmpty from 'lodash/isEmpty'; |
There was a problem hiding this comment.
And here, in this case we would see more benefit as we could truncate the 3 imports :D
Actually, this might be a good thing to tackle on another pass, regexing or code-modding for lodash/xx specifically.
jsnmoon
force-pushed
the
try/cjs-import-export-signup
branch
from
75c4925 to
31a802e
Compare
jsnmoon
force-pushed
the
try/cjs-import-export-signup
branch
from
31a802e to
988a918
Compare
drewblaisdell
force-pushed
the
try/cjs-import-export-signup
branch
from
988a918 to
9403f8d
Compare
Part of #11688.
This change will allow us to leverage Webpack 2's tree shaking optimization for ES6 modules in the future. Please share your suggestions and comments!