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

Unstable object nested in array #1203

Closed
vjeux opened this issue Apr 12, 2017 · 2 comments
Closed

Unstable object nested in array #1203

vjeux opened this issue Apr 12, 2017 · 2 comments
Labels
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! status:has pr Issues with an accompanying pull request. These issues will probably be fixed soon! type:bug Issues identifying ugly output, or a defect in the program
Milestone
1.0

Comments

@vjeux
Copy link
Contributor

vjeux commented Apr 12, 2017 

a(
  SomethingVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLong,
  [{SomethingVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLong: 1}],
)
a(
  SomethingVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLong,
  [
    {
      SomethingVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLong: 1
    }
  ]
);

The following looks really weird

a(
  SomethingVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLong,
  [{
      SomethingVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLong: 1
    }]
);

This is a regression from 0.22
@vjeux vjeux added the type:bug Issues identifying ugly output, or a defect in the program label Apr 12, 2017
@vjeux
Copy link
Contributor Author

vjeux commented Apr 12, 2017

This bug was introduced by #927

@vjeux
Copy link
Contributor Author

vjeux commented Apr 12, 2017

If I revert

concat(["(", join(concat([", "]), printedLastArgExpanded), ")"]),

to

concat(["(", join(concat([", "]), printed), ")"]),

then this problem goes away but it no longer put newlines correctly. I need to understand what's actually going on.

This was referenced Apr 12, 2017
Closed
Closed
@vjeux vjeux added the priority:high Code is printed in a way that alters the AST, breaks syntax, or is a significant regression. Urgent! label Apr 12, 2017
This was referenced Apr 12, 2017
Closed
Closed
@vjeux vjeux added this to the 1.0 milestone Apr 12, 2017
vjeux added a commit to vjeux/prettier that referenced this issue Apr 12, 2017
@vjeux
Fix edge cases triggered by newlines in arrow functions
9c3af10 
This one is pretty crazy. In prettier#927, I changed

```js
concat(["(", join(concat([", "]), printed), ")"]),
```

into

```js
concat(["(", join(concat([", "]), printedLastArgExpanded), ")"]),
```

which makes the example in prettier#1203 look ugly. The crazy thing is that `JSON.stringify(printed) === JSON.stringify(printedLastArgExpanded)`. So they are deep equal but not the same object. In a non-mutative world, this should cause any problem, but we actually mutate those to propagate breaks.

In the break propagation, we only looked at the first one in case of a conditional group. But, because the two were the same object then it also applied to the second one and happened to be the correct behavior! When I changed that piece of code to be two distinct objects, it no longer worked by accident and caused a bunch of weird issues where breaks didn't propagate correctly.

The solution for this case is to propagate the breaks on all the conditions. I'm pretty sure that this is the expected behavior, we want to deeply recurse in all of them, we don't propagate it up the boundary anyway.

The other use case for `traverseInDoc()` is `findInDoc()`, right now it searches for the first conditional group but it seems very arbitrary. I changed it to not search on any and none of the tests are failing, so I think it's safe(tm). If it triggers weird behavior, then it'll at least be expected and not randomly explode at us if we move groups around.

I tried to go through all the conditions for `findInDoc()` but it triggers a few failures (the output look bad). I'm not really sure why. https://gist.github.com/vjeux/5fb7566cc3d65974817d512d1ef6abe1

Fix prettier#1203
vjeux added a commit to vjeux/prettier that referenced this issue Apr 12, 2017
@vjeux
Fix edge cases triggered by newlines in arrow functions
736fd11 
This one is pretty crazy. In prettier#927, I changed

```js
concat(["(", join(concat([", "]), printed), ")"]),
```

into

```js
concat(["(", join(concat([", "]), printedLastArgExpanded), ")"]),
```

which makes the example in prettier#1203 look ugly. The crazy thing is that `JSON.stringify(printed) === JSON.stringify(printedLastArgExpanded)`. So they are deep equal but not the same object. In a non-mutative world, this should cause any problem, but we actually mutate those to propagate breaks.

In the break propagation, we only looked at the first one in case of a conditional group. But, because the two were the same object then it also applied to the second one and happened to be the correct behavior! When I changed that piece of code to be two distinct objects, it no longer worked by accident and caused a bunch of weird issues where breaks didn't propagate correctly.

The solution for this case is to propagate the breaks on all the conditions. I'm pretty sure that this is the expected behavior, we want to deeply recurse in all of them, we don't propagate it up the boundary anyway.

The other use case for `traverseInDoc()` is `findInDoc()`, right now it searches for the first conditional group but it seems very arbitrary. I changed it to not search on any and none of the tests are failing, so I think it's safe(tm). If it triggers weird behavior, then it'll at least be expected and not randomly explode at us if we move groups around.

I tried to go through all the conditions for `findInDoc()` but it triggers a few failures (the output look bad). I'm not really sure why. https://gist.github.com/vjeux/5fb7566cc3d65974817d512d1ef6abe1

Fix prettier#1203
@vjeux vjeux mentioned this issue Apr 12, 2017
Merged
@vjeux vjeux added the status:has pr Issues with an accompanying pull request. These issues will probably be fixed soon! label Apr 12, 2017
vjeux added a commit to vjeux/prettier that referenced this issue Apr 13, 2017
@vjeux
Fix edge cases triggered by newlines in arrow functions
be81c93 
This one is pretty crazy. In prettier#927, I changed

```js
concat(["(", join(concat([", "]), printed), ")"]),
```

into

```js
concat(["(", join(concat([", "]), printedLastArgExpanded), ")"]),
```

which makes the example in prettier#1203 look ugly. The crazy thing is that `JSON.stringify(printed) === JSON.stringify(printedLastArgExpanded)`. So they are deep equal but not the same object. In a non-mutative world, this should cause any problem, but we actually mutate those to propagate breaks.

In the break propagation, we only looked at the first one in case of a conditional group. But, because the two were the same object then it also applied to the second one and happened to be the correct behavior! When I changed that piece of code to be two distinct objects, it no longer worked by accident and caused a bunch of weird issues where breaks didn't propagate correctly.

The solution for this case is to propagate the breaks on all the conditions. I'm pretty sure that this is the expected behavior, we want to deeply recurse in all of them, we don't propagate it up the boundary anyway.

The other use case for `traverseInDoc()` is `findInDoc()`, right now it searches for the first conditional group but it seems very arbitrary. I changed it to not search on any and none of the tests are failing, so I think it's safe(tm). If it triggers weird behavior, then it'll at least be expected and not randomly explode at us if we move groups around.

I tried to go through all the conditions for `findInDoc()` but it triggers a few failures (the output look bad). I'm not really sure why. https://gist.github.com/vjeux/5fb7566cc3d65974817d512d1ef6abe1

Fix prettier#1203
vjeux added a commit that referenced this issue Apr 13, 2017
@vjeux
Fix edge cases triggered by newlines in arrow functions (#1217)
fe7ebc0 
This one is pretty crazy. In #927, I changed

```js
concat(["(", join(concat([", "]), printed), ")"]),
```

into

```js
concat(["(", join(concat([", "]), printedLastArgExpanded), ")"]),
```

which makes the example in #1203 look ugly. The crazy thing is that `JSON.stringify(printed) === JSON.stringify(printedLastArgExpanded)`. So they are deep equal but not the same object. In a non-mutative world, this should cause any problem, but we actually mutate those to propagate breaks.

In the break propagation, we only looked at the first one in case of a conditional group. But, because the two were the same object then it also applied to the second one and happened to be the correct behavior! When I changed that piece of code to be two distinct objects, it no longer worked by accident and caused a bunch of weird issues where breaks didn't propagate correctly.

The solution for this case is to propagate the breaks on all the conditions. I'm pretty sure that this is the expected behavior, we want to deeply recurse in all of them, we don't propagate it up the boundary anyway.

The other use case for `traverseInDoc()` is `findInDoc()`, right now it searches for the first conditional group but it seems very arbitrary. I changed it to not search on any and none of the tests are failing, so I think it's safe(tm). If it triggers weird behavior, then it'll at least be expected and not randomly explode at us if we move groups around.

I tried to go through all the conditions for `findInDoc()` but it triggers a few failures (the output look bad). I'm not really sure why. https://gist.github.com/vjeux/5fb7566cc3d65974817d512d1ef6abe1

Fix #1203
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 7, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
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! status:has pr Issues with an accompanying pull request. These issues will probably be fixed soon! type:bug Issues identifying ugly output, or a defect in the program
Projects
None yet
Milestone
1.0
Development

No branches or pull requests
1 participant
@vjeux