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.

Sign up for GitHub

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

Jump to bottom

Template expressions are incorrectly indented #216

Closed
kassens opened this issue Sep 22, 2015 · 9 comments · Fixed by prettier/prettier#218
Closed

Template expressions are incorrectly indented #216

kassens opened this issue Sep 22, 2015 · 9 comments · Fixed by prettier/prettier#218

Comments

@kassens
Copy link

kassens commented Sep 22, 2015

If a template expression is printed that's indented, it seems like indentation is added twice somewhere. This failing test illustrates the bug.

it("preserves indentation in template expressions", function() {
    var printer = new Printer({
        tabWidth: 2
    });
    code = [
        "var x = {",
        "  y: () => Relay.QL`",
        "    query {",
        "      ${foo},",
        "      field,",
        "    }",
        "  `",
        "};",
    ].join("\n");

    ast = parse(code);
    pretty = printer.printGenerically(ast).code;
    assert.strictEqual(pretty, code);
});

Output

-actual
+expected

 var x = {
   y: () => Relay.QL`
-      query {
-        ${foo},
-        field,
-      }
-    `
+    query {
+      ${foo},
+      field,
+    }
+  `
 };
@kassens
Copy link
Author

kassens commented Sep 22, 2015

cc @fkling, @cpojer

@benjamn
Copy link
Owner

benjamn commented Sep 22, 2015

Oh, interesting. I'm not sure the spaces inside the back-ticks are even considered as indentation, as far as Recast is currently concerned. So part of the double indentation is coming from the pretty printer, and the other half is coming from the original template string expression.

GraphQL template strings can be reindented harmlessly, but I think in general we need to avoid adding or removing "indentation" from multi-line template strings.

@kassens
Copy link
Author

kassens commented Sep 22, 2015

A heuristic that could be used: if all lines are indented, assume it's some sort of markup and okay to re-indent.

Preserving the same indent would already be better and solve most cases (when changes don't affect indentation).

@yungsters
Copy link

Started looking into this again: http://felix-kling.de/esprima_ast_explorer/#/N2D8hbBQ3I/1

benjamn added a commit that referenced this issue Nov 11, 2015
@benjamn
Add a failing test for issue #216.
68392a1
@kassens
Copy link
Author

kassens commented Nov 11, 2015

Thanks!

@yungsters
Copy link

Looks like this still exhibits a bug after 9e6fdb8: http://felix-kling.de/esprima_ast_explorer/#/N2D8hbBQ3I/1

(I tested using a local install of jscodeshift using recast@0.10.38.)

@yungsters
Copy link

@benjamn Should I file a new issue for this?

benjamn added a commit that referenced this issue Nov 19, 2015
@benjamn
Add a failing test of reprinting modified tagged template expressions.
4db38fe 
Reopens #216.
@benjamn benjamn reopened this Nov 19, 2015
benjamn added a commit that referenced this issue Nov 20, 2015
@benjamn
Fix faulty .loc information for children of TemplateLiteral nodes.
f45c3c0 
Helps fix #216.
@benjamn
Copy link
Owner

benjamn commented Nov 20, 2015

This was still broken due to a combination of incomplete indentation-locking logic (4de9390) and some bad .loc information for TemplateElement nodes (f45c3c0).

Just published v0.10.39 to NPM: https://www.npmjs.com/package/recast

vjeux added a commit to vjeux/prettier that referenced this issue Jan 15, 2017
@vjeux
Remove faulty location check on template literals that throws in Nuclide
25054ba 
There's a handful of files inside of Nuclide that throw exceptions because an assertion is raised.

```
{ AssertionError: ']' === '`'
    at fixTemplateLiteral (/Users/vjeux/random/prettier/src/util.js:105:10)
    at Object.util.fixFaultyLocations (/Users/vjeux/random/prettier/src/util.js:45:5)
    at getSortedChildNodes (/Users/vjeux/random/prettier/src/comments.js:25:8)
    at getSortedChildNodes (/Users/vjeux/random/prettier/src/comments.js:61:5)
    at decorateComment (/Users/vjeux/random/prettier/src/comments.js:71:20)
    at decorateComment (/Users/vjeux/random/prettier/src/comments.js:85:7)
    at decorateComment (/Users/vjeux/random/prettier/src/comments.js:85:7)
    at decorateComment (/Users/vjeux/random/prettier/src/comments.js:85:7)
    at decorateComment (/Users/vjeux/random/prettier/src/comments.js:85:7)
    at /Users/vjeux/random/prettier/src/comments.js:129:5
```

When trying https://github.com/facebook/nuclide/blob/master/pkg/nuclide-task-runner/lib/main.js#L174

It throws in the fixTemplateLiteral method.

That method was added to fix benjamn/recast#216 more than a year ago

```js
var x = {
  y: () => Relay.QL`
    query {
      ${foo},
      field,
    }
  `
};
```

I've checked (and added a test) and it now parses and prints correctly without that method. So it should be safe to remove.
vjeux added a commit to vjeux/prettier that referenced this issue Jan 15, 2017
@vjeux
Remove faulty location check on template literals that throws in Nuclide
20866fd 
There's a handful of files inside of Nuclide that throw exceptions because an assertion is raised.

```
{ AssertionError: ']' === '`'
    at fixTemplateLiteral (/Users/vjeux/random/prettier/src/util.js:105:10)
    at Object.util.fixFaultyLocations (/Users/vjeux/random/prettier/src/util.js:45:5)
    at getSortedChildNodes (/Users/vjeux/random/prettier/src/comments.js:25:8)
    at getSortedChildNodes (/Users/vjeux/random/prettier/src/comments.js:61:5)
    at decorateComment (/Users/vjeux/random/prettier/src/comments.js:71:20)
    at decorateComment (/Users/vjeux/random/prettier/src/comments.js:85:7)
    at decorateComment (/Users/vjeux/random/prettier/src/comments.js:85:7)
    at decorateComment (/Users/vjeux/random/prettier/src/comments.js:85:7)
    at decorateComment (/Users/vjeux/random/prettier/src/comments.js:85:7)
    at /Users/vjeux/random/prettier/src/comments.js:129:5
```

When trying https://github.com/facebook/nuclide/blob/master/pkg/nuclide-task-runner/lib/main.js#L174

It throws in the fixTemplateLiteral method.

That method was added to fix benjamn/recast#216 more than a year ago

```js
var x = {
  y: () => Relay.QL`
    query {
      ${foo},
      field,
    }
  `
};
```

I've checked (and added a test) and it now parses and prints correctly without that method. So it should be safe to remove.
@vjeux vjeux mentioned this issue Jan 15, 2017
Merged
vjeux added a commit to vjeux/prettier that referenced this issue Jan 16, 2017
@vjeux
Remove faulty location check on template literals that throws in Nuclide
ee23df5 
There's a handful of files inside of Nuclide that throw exceptions because an assertion is raised.

```
{ AssertionError: ']' === '`'
    at fixTemplateLiteral (/Users/vjeux/random/prettier/src/util.js:105:10)
    at Object.util.fixFaultyLocations (/Users/vjeux/random/prettier/src/util.js:45:5)
    at getSortedChildNodes (/Users/vjeux/random/prettier/src/comments.js:25:8)
    at getSortedChildNodes (/Users/vjeux/random/prettier/src/comments.js:61:5)
    at decorateComment (/Users/vjeux/random/prettier/src/comments.js:71:20)
    at decorateComment (/Users/vjeux/random/prettier/src/comments.js:85:7)
    at decorateComment (/Users/vjeux/random/prettier/src/comments.js:85:7)
    at decorateComment (/Users/vjeux/random/prettier/src/comments.js:85:7)
    at decorateComment (/Users/vjeux/random/prettier/src/comments.js:85:7)
    at /Users/vjeux/random/prettier/src/comments.js:129:5
```

When trying https://github.com/facebook/nuclide/blob/master/pkg/nuclide-task-runner/lib/main.js#L174

It throws in the fixTemplateLiteral method.

That method was added to fix benjamn/recast#216 more than a year ago

```js
var x = {
  y: () => Relay.QL`
    query {
      ${foo},
      field,
    }
  `
};
```

I've checked (and added a test) and it now parses and prints correctly without that method. So it should be safe to remove.
jlongster pushed a commit to prettier/prettier that referenced this issue Jan 16, 2017
@vjeux @jlongster
Remove faulty location check on template literals that throws in Nucl…
c5d7619 
…ide (#218)

There's a handful of files inside of Nuclide that throw exceptions because an assertion is raised.

```
{ AssertionError: ']' === '`'
    at fixTemplateLiteral (/Users/vjeux/random/prettier/src/util.js:105:10)
    at Object.util.fixFaultyLocations (/Users/vjeux/random/prettier/src/util.js:45:5)
    at getSortedChildNodes (/Users/vjeux/random/prettier/src/comments.js:25:8)
    at getSortedChildNodes (/Users/vjeux/random/prettier/src/comments.js:61:5)
    at decorateComment (/Users/vjeux/random/prettier/src/comments.js:71:20)
    at decorateComment (/Users/vjeux/random/prettier/src/comments.js:85:7)
    at decorateComment (/Users/vjeux/random/prettier/src/comments.js:85:7)
    at decorateComment (/Users/vjeux/random/prettier/src/comments.js:85:7)
    at decorateComment (/Users/vjeux/random/prettier/src/comments.js:85:7)
    at /Users/vjeux/random/prettier/src/comments.js:129:5
```

When trying https://github.com/facebook/nuclide/blob/master/pkg/nuclide-task-runner/lib/main.js#L174

It throws in the fixTemplateLiteral method.

That method was added to fix benjamn/recast#216 more than a year ago

```js
var x = {
  y: () => Relay.QL`
    query {
      ${foo},
      field,
    }
  `
};
```

I've checked (and added a test) and it now parses and prints correctly without that method. So it should be safe to remove.
@benjamn benjamn mentioned this issue Aug 5, 2017
Open
@nickretallack
Copy link

This is still broken. I originally filed an issue here: zxbodya/flowts#8

It seems to be looking at the first line of the template string and stripping that much indentation from all later lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove faulty location check on template literals that throws in Nuclide vjeux/prettier
4 participants
@benjamn @kassens @nickretallack @yungsters