Skip to content

Conversation

@bcoe
Copy link
Member

@bcoe bcoe commented May 30, 2018

No description provided.

@coveralls
Copy link

coveralls commented May 30, 2018

Coverage Status

Coverage increased (+0.02%) to 88.058% when pulling 97634e7 on upgrade-babel into ac824a4 on master.

@bcoe
Copy link
Member Author

bcoe commented May 30, 2018

CC: @hzoo, @loganfsmyth, how is this upgrade looking? I want to be fairly careful not to break the world.

@bcoe
Copy link
Member Author

bcoe commented May 30, 2018

  static get({
             ^
SyntaxError: Unexpected token {
    at exports.runInThisContext (vm.js:53:16)
    at Module._compile (module.js:373:25)
    at Module.replacementCompile (/home/travis/build/istanbuljs/istanbuljs/node_modules/nyc/node_modules/append-transform/index.js:58:13)```

☝️I guess this means I'd need to drop v4 support?

Copy link

@loganfsmyth loganfsmyth left a 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",

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: [
"*",

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",

Choose a reason for hiding this comment

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

Did this get missed?

@bcoe
Copy link
Member Author

bcoe commented May 30, 2018

@loganfsmyth it looks like Node 4 support is dropped in Babel@7?

@loganfsmyth
Copy link

@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",

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.

Copy link
Member Author

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 truthy

is there an additional plugin that's required? seems to be a regression from [email protected].

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.

Copy link
Member Author

@bcoe bcoe May 30, 2018

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.

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}';
that means the interpreter directive is no longer at the start of the file, and thus a syntax error.

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?

@bcoe bcoe merged commit ce23e91 into master May 31, 2018
@bcoe bcoe deleted the upgrade-babel branch May 31, 2018 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants