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.

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

Do not mutate ast #8342

Merged
merged 1 commit into from Jul 24, 2018

Conversation

@thiagoarrais
Copy link
Contributor

thiagoarrais commented Jul 19, 2018

Q                       A
Fixed Issues? Fixes #7987
Patch: Bug Fix? 👍
Major: Breaking Change? 👎
Minor: New Feature? 👎
Tests Added + Pass? 👍
Documentation PR Link
Any Dependency Changes? 👎
License MIT

This clones the given ast prior to working on it to avoid mutating the AST given to transformFromAST.

This is work in progress and intended as input for review. Some questions I have:

  • Is this the intended use for t.cloneNode?
  • parse.js was the easiest place to insert the test. I'm not sure where it should go

Please check inline review comments for context.

@thiagoarrais thiagoarrais force-pushed the thiagoarrais:do-not-mutate-ast branch from a856a18 to 5825e43 Jul 19, 2018
if (ast) {
if (ast.type === "Program") {
ast = t.file(ast, [], []);
} else if (ast.type !== "File") {
throw new Error("AST root must be a Program or File node");
}
ast = t.cloneNode(ast);

This comment has been minimized.

Copy link
@thiagoarrais

thiagoarrais Jul 19, 2018

Author Contributor

Is this the intended use for t.cloneNode?

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Jul 20, 2018

Member

The problem is that cloneNode only clones properties which represents other nodes. It doesn't copy every property (e.g. position, extras...).

I would use lodash's cloneDeep function.

This comment has been minimized.

Copy link
@thiagoarrais

thiagoarrais Jul 20, 2018

Author Contributor

Good idea. Will take a look at that.

transformFromAst(node, program, {
filename: fixture("input.js"),
cwd: fixture(),
plugins: [

This comment has been minimized.

Copy link
@thiagoarrais

thiagoarrais Jul 19, 2018

Author Contributor

the purpose of this inline plugin is to force the AST to change. any ideas on how to write it more succinctly?

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Jul 20, 2018

Member

You can replace function() { return with () => (, but it is not really important

This comment has been minimized.

Copy link
@thiagoarrais

thiagoarrais Jul 20, 2018

Author Contributor

Yup. I was wondering if you folks already have some established helper or something for this kind of stuff.

@@ -28,4 +28,32 @@ describe("parse", function() {
});
expect(JSON.parse(JSON.stringify(result))).toEqual(output);
});

This comment has been minimized.

Copy link
@thiagoarrais

thiagoarrais Jul 19, 2018

Author Contributor

parse.js was the easiest place to insert this test. I'm not sure where it should go. My first instinct was to add it to test/transformation.js, but there weren't any describe'd tests there...

@thiagoarrais thiagoarrais changed the title Do not mutate ast [WIP] Do not mutate ast Jul 20, 2018
@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Jul 20, 2018

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/8668/

Clone it prior to processing
@thiagoarrais thiagoarrais force-pushed the thiagoarrais:do-not-mutate-ast branch from 6a27775 to 496e522 Jul 20, 2018
@thiagoarrais

This comment has been minimized.

Copy link
Contributor Author

thiagoarrais commented Jul 20, 2018

I think this is good to go now. I'm removing the WIP marker.

@thiagoarrais thiagoarrais changed the title [WIP] Do not mutate ast Do not mutate ast Jul 20, 2018
@thiagoarrais

This comment has been minimized.

Copy link
Contributor Author

thiagoarrais commented Jul 20, 2018

Closing and reopening to try and trigger a Travis rebuild.

});

expect(code).toBe("const replaced = 1;");
expect(node.program.body[0].declarations[0].id.name).toBe(

This comment has been minimized.

Copy link
@hzoo

hzoo Jul 21, 2018

Member

Since there was a discussion on deep cloning the test (or a new test) should probably reflect that, like check the the other metadata fields aren't changed?

This comment has been minimized.

Copy link
@thiagoarrais

thiagoarrais Jul 23, 2018

Author Contributor

That would be nice, yes. Any ideas on how to force a metadata change? I'm outputing the AST for this transform and it doesn't seem to change any metadata fields...

This comment has been minimized.

Copy link
@hzoo

hzoo Jul 23, 2018

Member

I don't think you can other than just setting some value manually like path.node.x? Maybe this test is enough since the replace is already deeply nested above the initial program?

This comment has been minimized.

Copy link
@thiagoarrais

thiagoarrais Jul 23, 2018

Author Contributor

I can add something like that if you folks think it would be useful. I am not really in a good position to evaluate that, though. Do people usually sprinkle such metadata fields into their ASTs?

This comment has been minimized.

Copy link
@hzoo

hzoo Jul 24, 2018

Member

Actually this works fine! thanks!

@hzoo
hzoo approved these changes Jul 23, 2018
@hzoo hzoo merged commit 6f3a800 into babel:master Jul 24, 2018
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 80.47% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lock lock bot added the outdated label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.