Fix hoisting of "var" variables on wrapped modules #1735
Conversation
| @@ -135,6 +135,36 @@ module.exports = { | |||
| ) { | |||
| scope.push({id: exportsIdentifier, init: t.objectExpression([])}); | |||
| } | |||
|
|
|||
| // Move all "var" variables to the top-level to prevent out of order definitions when wrapped. | |||
| for (let name in scope.bindings) { | |||
There was a problem hiding this comment.
hmm yeah we do something like this here already: https://github.com/parcel-bundler/parcel/blob/master/src/packagers/JSConcatPackager.js#L282-L319 but it wouldn't get variables inside an if statement like that.
Is there a performance hit to doing this on every module? Should we only do it on wrapped modules (in the packager)?
There was a problem hiding this comment.
Is there a performance hit to doing this on every module?
I did not found any noticeable change on build time on a medium size app, same for vue-hackernews. We only do it when a var variable is not in the root block of the current scope, which is relatively rare.
Should we only do it on wrapped modules (in the packager)?
I guess you could either :
- build a scope using
babel-traverseand do the same as here => slow - do a deep AST traversal => slow and error prone
Just found out other some issues with this PR, and I think I should also do it when path.getData('shouldWrap') == true, I'm putting it in WIP.
There was a problem hiding this comment.
Looks like we don't need to do it when shouldWrap is true, ready to merge!
fathyb
changed the title
fathyb
changed the title
|
This breaks React, closing it. |
Part of the fixes for #1699
Fix issues with some React apps. One of the problematic files (
loggedTypeFailures) :https://github.com/facebook/prop-types/blob/cfc7f43af3d74978ec82052fe6bb8212f09daca0/checkPropTypes.js