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

[babel-generator] Add check for undefined endToken in whitespace #7205

Merged
merged 2 commits into from
Feb 3, 2018

Conversation

MatthieuLemoine
Copy link

Q A
Fixed Issues? None
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? No
Documentation PR No
Any Dependency Changes? No
License MIT

Hi ! 👋

In some weird cases, in the following method in src/whitespace.js :

  getNewlinesAfter(node) {
    let startToken;
    let endToken;
    const tokens = this.tokens;

    let index = this._findToken((token) => token.end - node.end, 0, tokens.length);
    if (index >= 0) {
      while (index && node.end === tokens[index - 1].end) --index;
      startToken = tokens[index];
      endToken = tokens[index + 1];
     // endToken can be undefined if startToken is EOF
      if (endToken.type.label === ",") endToken = tokens[index + 2];
    }

    if (endToken && endToken.type.label === "eof") {
      return 1;
    } else {
      return this._getNewlinesBetween(startToken, endToken);
    }
  }

startToken can be an EOF which leads to endToken being undefined and the following error :

error: bundling failed: TypeError: Cannot read property 'type' of undefined
    at Whitespace.getNewlinesAfter (/home/user/app/node_modules/babel-generator/lib/whitespace.js:59:20)
    at Generator._printNewline (/home/user/app/node_modules/babel-generator/lib/printer.js:459:34)
    at Generator.printJoin (/home/user/app/node_modules/babel-generator/lib/printer.js:376:32)
    at Generator.printList (/home/user/app/node_modules/babel-generator/lib/printer.js:430:17)
    at Generator.ObjectExpression (/home/user/app/node_modules/babel-generator/lib/generators/types.js:57:10)
    at /home/user/app/node_modules/babel-generator/lib/printer.js:298:23
    at Buffer.withSource (/home/user/app/node_modules/babel-generator/lib/buffer.js:168:5)
    at Generator.withSource (/home/user/app/node_modules/babel-generator/lib/printer.js:189:15)
    at Generator.print (/home/user/app/node_modules/babel-generator/lib/printer.js:297:10)

To avoid this issue, this PR adds an undefined check.

You can look into this issue for reference : detrohutt/babel-plugin-import-graphql#2.

You can find a small reproduction and a video showing this weird behavior.

@rajasekarm
Copy link
Member

Can you add test case for the same?

@MatthieuLemoine
Copy link
Author

@rajzshkr Yes but I don't know what kind of tests I can add as there's no unit test for this method and I don't have a deep understanding of babel-generator's internals.

@rajasekarm
Copy link
Member

rajasekarm commented Jan 13, 2018

check this test case, https://github.com/babel/babel/blob/master/packages/babel-generator/test/fixtures/auto-indentation/hard-tab. Make your failing code in input.js and make sure you get the required output.

@nicolo-ribaudo
Copy link
Member

@MatthieuLemoine Does this bug also affect Babel 7? If yes, please make your PR target that branch. In case it has alread been fixed, we probably won't release a new Babel 6 version.

@MatthieuLemoine
Copy link
Author

MatthieuLemoine commented Jan 13, 2018

@nicolo-ribaudo I don't think that this bug affects Babel 7 as getNewlinesAfter has been removed from the codebase.

Tokens have been removed from babel-generator

@MatthieuLemoine
Copy link
Author

Upgrading to Babel 7 is not straightforward as a lot of plugins has not been updated to work with Babel 7. So I hope that a new bug fix release will be made.

@MatthieuLemoine
Copy link
Author

Test case added.

@MatthieuLemoine
Copy link
Author

@nicolo-ribaudo As you can see, a lot of people is experiencing this issue.

This PR only adds a missing undefined check. It looks like an oversight as there's already an undefined check two lines below :

if (endToken && endToken.type.label === "eof")

If Babel 6 is still officially supported, a bug fix release should be possible.

@gihrig
Copy link

gihrig commented Feb 1, 2018

I have been running into this issue on a daily basis - very annoying!

A Babel 6 release would be awesome! ✨

@Andarist
Copy link
Member

Andarist commented Feb 2, 2018

@MatthieuLemoine if we can make CircleCI to pass (I've restarted the build now to see if that helps) - then we should be able to prepare a bug fix release.

@babel-bot
Copy link
Collaborator

babel-bot commented Feb 2, 2018

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

@hzoo hzoo merged commit 036e664 into babel:6.x Feb 3, 2018
@hzoo
Copy link
Member

hzoo commented Feb 3, 2018

Hope this is understandable but we're not going to be doing this often as the last 6.x release was 6 months ago, and probably won't be doing it at all once 7.x is out unless it's sponsored. We have been working on 7.x for probably a year now and would like to focus on releasing that and fixing any issues people have with upgrading that instead of backporting forever, especially with the limited time people have. No one is really funding the development, especially for something like a backport which is a time consuming thing to do for a volunteer team, let alone a full time sponsored project.

Also had a lot of issues with releasing 6.x in the past due to publishing issues in our old setup so I have to go back to figure out why that is and then might cause even more issues for others so it's another reason why extremely hesitate to land anything #4947

https://medium.com/@mikeal/stop-supporting-old-releases-70cfa0e04b0c

I am going to update the readme's so we can finally switch our docs over and provide an upgrade experience for 7.x while allowing for switching to the 6.x docs babel/website#1544 so might as well add this in since it's unlikely this will cause any issues unlike a new feature request or port on 6.x

Made https://github.com/babel/babel/releases/tag/v6.26.1, it only updates that packages so you'll want to remove node_modules/redo the lock file so that the only deps pick it up

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
6.x: backport outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants