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.

Sign up for GitHub

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

Jump to bottom

Scope hoisting renaming after babel transforms #2292

Merged
merged 3 commits into from Nov 18, 2018
Merged

Scope hoisting renaming after babel transforms #2292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e646403
Fix superclass scope hoisting
mischnic Nov 17, 2018
09c6564
Add test for jsx scope hoisting
mischnic Nov 17, 2018
fa9ef65
Change back renamer and call crawl instead
mischnic Nov 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/core/parcel-bundler/src/scope-hoisting/hoist.js
View file
Open in desktop
Expand Up @@ -51,6 +51,8 @@ function hasSideEffects(asset, {sideEffects} = asset._package) {
module.exports = {
Program: {
enter(path, asset) {
path.scope.crawl();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get why we need this, isn't babel traverse going to .crawl for us already?

Copy link
Member Author

@mischnic mischnic Nov 18, 2018 •
edited

Choose a reason for hiding this comment

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

Here is an explanation: #1699 (comment)

It seems that when JSX is transformed (calling babel.transformFromAst(asset.ast, asset.contents, config)), the scope binding is not updated (only AST is updated). So in the hoist phase, we fail to find binding of React because there is still JSX syntax in the scope.

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 babel doesn't recrawl the scope automatically, only when the scope is first initialized. That happens before lots of babel transforms run which can modify references, in this case the class transform. Then we get to our hoisting transform and we need to re-crawl to make sure we have updated references since babel traverse doesn't do that automatically.


asset.cacheData.imports = asset.cacheData.imports || Object.create(null);
asset.cacheData.exports = asset.cacheData.exports || Object.create(null);
asset.cacheData.wildcards = asset.cacheData.wildcards || [];
Expand Down
4 changes: 2 additions & 2 deletions packages/core/parcel-bundler/src/scope-hoisting/renamer.js
View file
Open in desktop
Expand Up @@ -25,9 +25,9 @@ function rename(scope, oldName, newName) {
}

// Rename binding identifier, and update scope.
binding.identifier.name = newName;
scope.removeOwnBinding(oldName);
scope.bindings[newName] = binding;
delete scope.bindings[oldName];
binding.identifier.name = newName;
}

module.exports = rename;
3 changes: 3 additions & 0 deletions packages/core/parcel-bundler/test/integration/scope-hoisting/es6/jsx-pragma/.babelrc
View file
Open in desktop
@@ -0,0 +1,3 @@
{
"plugins": ["@babel/plugin-transform-react-jsx"]
}
3 changes: 3 additions & 0 deletions packages/core/parcel-bundler/test/integration/scope-hoisting/es6/jsx-pragma/a.js
View file
Open in desktop
@@ -0,0 +1,3 @@
import React from "./react.js";

output = <span>Test</span>;
9 changes: 9 additions & 0 deletions packages/core/parcel-bundler/test/integration/scope-hoisting/es6/jsx-pragma/react.js
View file
Open in desktop
@@ -0,0 +1,9 @@
// mock for React.createElement

export default {
createElement(type, props, children){
return {
type, props, children
};
}
}
5 changes: 5 additions & 0 deletions packages/core/parcel-bundler/test/integration/scope-hoisting/es6/rename-superclass/a.js
View file
Open in desktop
@@ -0,0 +1,5 @@
import Superclass from './b';

class Test extends Superclass {}

output = new Test().parentMethod();
5 changes: 5 additions & 0 deletions packages/core/parcel-bundler/test/integration/scope-hoisting/es6/rename-superclass/b.js
View file
Open in desktop
@@ -0,0 +1,5 @@
export default class Superclass {
parentMethod(){
return 2;
}
}
24 changes: 24 additions & 0 deletions packages/core/parcel-bundler/test/scope-hoisting.js
View file
Open in desktop
Expand Up @@ -89,6 +89,17 @@ describe('scope hoisting', function() {
assert.equal(output, 2);
});

it('supports renaming superclass identifiers', async function() {
let b = await bundle(
path.join(
__dirname,
'/integration/scope-hoisting/es6/rename-superclass/a.js'
)
);
let output = await run(b);
assert.equal(output, 2);
});

it('supports renaming imports', async function() {
let b = await bundle(
path.join(
Expand Down Expand Up @@ -446,6 +457,19 @@ describe('scope hoisting', function() {
assert.deepEqual(output, 'bar');
});

it('should support the jsx pragma', async function() {
let b = await bundle(
path.join(__dirname, '/integration/scope-hoisting/es6/jsx-pragma/a.js')
);

let output = await run(b);
assert.deepEqual(output, {
children: 'Test',
props: null,
type: 'span'
});
});

it('should not nameclash with internal variables', async function() {
let b = await bundle(
path.join(__dirname, '/integration/scope-hoisting/es6/name-clash/a.js')
Expand Down