-
-
Notifications
You must be signed in to change notification settings - Fork 246
chore: upgrade babel in instrumenter #174
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
Conversation
|
CC: @hzoo, @loganfsmyth, how is this upgrade looking? I want to be fairly careful not to break the world. |
|
loganfsmyth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable overall.
| "babel-generator": "^6.18.0", | ||
| "babel-template": "^6.16.0", | ||
| "babel-traverse": "^6.18.0", | ||
| "@babel/generator": "^7.0.0-beta.49", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't use ^ in any of these versions since you never know if we'll make any final breaking changes that may or may not affect this.
| sourceType: "script", // I think ? | ||
| plugins: ["*"] | ||
| plugins: [ | ||
| "*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parser doesn't have * anymore. Some parser plugins conflict with others, so we require an explicit list of parser plugins to enable.
| "@babel/generator": "^7.0.0-beta.49", | ||
| "@babel/template": "^7.0.0-beta.49", | ||
| "@babel/traverse": "^7.0.0-beta.49", | ||
| "babel-types": "^6.18.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this get missed?
|
@loganfsmyth it looks like Node 4 support is dropped in Babel@7? |
|
@bcoe Yes it is, so that's a good point. We've doing >= 6.9 for Babel 7. |
| "@babel/template": "7.0.0-beta.49", | ||
| "@babel/traverse": "7.0.0-beta.49", | ||
| "@babel/types": "7.0.0-beta.49", | ||
| "babylon": "7.0.0-beta.47", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh we renamed, which is why the versions don't match. It's just @babel/parser now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting, switching to @babel/parser seems to break shebang support:
1) statement.yaml/shebang code
covers line and one branch:
AssertionError: Error compiling
1: #!/usr/bin/env node
2:
3: var x = args[0] > 5 ? args[0] : "undef";
4: output = x;
5:
Invalid or unexpected token: expected false to be truthyis there an additional plugin that's required? seems to be a regression from [email protected].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm interesting. I'll take a look. I did land a change for shebang handling, but I can't tell from that error if it's a bug, or just not expecting the new logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting, it doesn't seem to be shebangs in general, this works:
// #!/usr/bin/env node
var x = args[0] > 5 ? args[0] : "undef";
output = x;but this doesn't work:
#!/usr/bin/env node
var x = args[0] > 5 ? args[0] : "undef";
output = x;update: I only seem to have one test that uses a shebang.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I see what's up. In Babel 6 the interpreter directive was basically silently converted into a LineComment in the AST.
In 7.x, we changed is that now if your input file had a shebang, it actually gets passed to the code generator and generated as a shebang again. That's causing problems for your logic here:
| wrapped = '{ var output;\n' + instrumenterOutput + '\nreturn output;\n}'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe your tests should just strip directives in that logic?
No description provided.