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

Dynamic import with webpackChunkName comment fails #1489

Closed
pbomb opened this issue May 2, 2017 · 8 comments
Closed

Dynamic import with webpackChunkName comment fails #1489

pbomb opened this issue May 2, 2017 · 8 comments
Labels
area:comments Issues with how Prettier prints comments locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. priority:high Code is printed in a way that alters the AST, breaks syntax, or is a significant regression. Urgent! type:bug Issues identifying ugly output, or a defect in the program

Comments

@pbomb
Copy link

pbomb commented May 2, 2017

Prettier dies when trying to format a file that has a dynamic import with a webpackChunkName magic comment as described in the Webpack docs. For example, the following code:

const foo =  import(/* webpackChunkName: "foo" */ './foo');

fails with this error:

src/client/pages/trace/Page.js: Error: Comment "webpackChunkName: "foo"" was not printed. Please report this error!
    at astComments.forEach.comment (/Users/parma36/projects/hugo/node_modules/prettier/index.js:69:13)
    at Array.forEach (native)
    at ensureAllCommentsPrinted (/Users/parma36/projects/hugo/node_modules/prettier/index.js:67:15)
    at format (/Users/parma36/projects/hugo/node_modules/prettier/index.js:85:3)
    at formatWithShebang (/Users/parma36/projects/hugo/node_modules/prettier/index.js:91:12)
    at Object.format (/Users/parma36/projects/hugo/node_modules/prettier/index.js:105:12)
    at format (/Users/parma36/projects/hugo/node_modules/prettier/bin/prettier.js:234:19)
    at err (/Users/parma36/projects/hugo/node_modules/prettier/bin/prettier.js:328:16)
    at filenames.forEach.filename (/Users/parma36/projects/hugo/node_modules/prettier/bin/prettier.js:385:9)
    at Array.forEach (native)

This is happening with Prettier v1.3.0

https://prettier.github.io/prettier/#%7B%22content%22%3A%22const%20foo%20%3D%20%20import(%2F*%20webpackChunkName%3A%20%5C%22foo%5C%22%20*%2F%20'.%2Ffoo')%3B%22%2C%22options%22%3A%7B%22printWidth%22%3A80%2C%22tabWidth%22%3A2%2C%22singleQuote%22%3Afalse%2C%22trailingComma%22%3A%22%22%2C%22bracketSpacing%22%3Atrue%2C%22jsxBracketSameLine%22%3Afalse%2C%22parser%22%3A%22flow%22%2C%22semi%22%3Afalse%2C%22useTabs%22%3Afalse%2C%22doc%22%3Afalse%7D%7D

@vjeux vjeux added type:bug Issues identifying ugly output, or a defect in the program area:comments Issues with how Prettier prints comments priority:high Code is printed in a way that alters the AST, breaks syntax, or is a significant regression. Urgent! labels May 2, 2017
@vjeux
Copy link
Contributor

vjeux commented May 2, 2017

Thanks for reporting this error! We need to fix it. In the meantime, there are a few options for you:

  • You can use babylon parser (the default) instead of flow which doesn't have this issue
  • You can add // prettier-ignore in the line before
  • You can remove the comment

@pbomb
Copy link
Author

pbomb commented May 2, 2017

Great, thanks. I thought I had to use the flow parser if I was using flow. The babylon parser does seem to work just fine, so I'll switch over to using that. Thanks!

@vjeux
Copy link
Contributor

vjeux commented May 3, 2017

Yeah, sorry about that, it's confusing a lot of people and we're not doing a good job at explaining. Both parsers support the same set of language features, the main reason to have both is mostly insurance. So if one gets behind, we have a fallback. Good news is that both teams are very responsive on issues :)

@vjeux
Copy link
Contributor

vjeux commented May 22, 2017

For some reason, the traversal logic to find comment doesn't go through the same path where the name of CallExpression is of type Literal vs Import. What happens currently is that it attaches the comment as a dangling comment to the Import node, which doesn't know how to print it.

@rkurbatov
Copy link
Contributor

rkurbatov commented Sep 4, 2017

I started digging into this issue and took minimal example:

import(/* Hello */'string')

vs

prompt(/* Hello */'string')

They are parsed completely differently by flow-parser:

"callee": {
          "type": "Import",
          "loc": {
            "source": null,
            "start": {
              "line": 1,
              "column": 0
            },
            "end": {
              "line": 1,
              "column": 26
            }
          },
          "range": [
            0,
            26
          ]
        },

vs

"callee": {
          "type": "Identifier",
          "loc": {
            "source": null,
            "start": {
              "line": 1,
              "column": 0
            },
            "end": {
              "line": 1,
              "column": 6
            }
          },
          "range": [
            0,
            6
          ],
          "name": "prompt",
          "typeAnnotation": null,
          "optional": false
        },

Thus comment is contained by Import node with no preceding and following node while in prompt case it has Identifier and Literal as preceding and following.
I cannot write special handler as (in any case) I have only enclosingNode so comment attaching can be done only before or after 'import' function name. It seems like flow-parser bug that requires special handling by decorateComment function and that's too much as for me.

@rkurbatov
Copy link
Contributor

btw, another case

import('string'/* Hello */)

doesn't throw but it has wrong output:

import('string') /* Hello */;

Here enclosingNode is ExpressionStatement and precedingNode is CallExpression instead of Literal.

@rkurbatov
Copy link
Contributor

rkurbatov commented Sep 4, 2017

It seems like I fixed this issue in #2748 but some cases are not handled:

import('something'
// Hello
)

is working but output is

import('something');
// Hello

Is this ok?

@azz
Copy link
Member

azz commented Sep 5, 2017

That's fine, much better than how we were previously handling it. Thanks!

@azz azz closed this as completed Sep 5, 2017
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 6, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area:comments Issues with how Prettier prints comments locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. priority:high Code is printed in a way that alters the AST, breaks syntax, or is a significant regression. Urgent! type:bug Issues identifying ugly output, or a defect in the program
Projects
None yet
Development

No branches or pull requests

4 participants