Skip to content

Conversation

@benjie
Copy link

@benjie benjie commented Sep 4, 2019

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

I'm working on flow2typescript and trying to match the input source code as best as I can. I'm using the retainLines feature to position the comments accurately. This works great, for the most part, but there's a number of issues with closing braces and brackets appearing on the previous line.

This PR uses the withSource helper to allow the printer to "catch up" for the closing brace in many cases. I've added a test for this; here's the test result without the changes:

  ● generation/typescript › class properties with retainlines option

    expect(received).toBe(expected) // Object.is equality

    - Expected
    + Received

      class ClassName {
        a: {
          a1: number;
    -     a2: number;
    -   };
    +     a2: number;};
    + 
        b: {
          b1: number;
    -     b2: number;
    -   };
    +     b2: number;};
    + 
        c(
        a: number,
        b: number,
        c: number,
        d: {
          d1: number;
          d2: number;
    -     d3: number;
    -   })
    +     d3: number;})
    + 
        : {
          a: number;
          b: number;
          c: number;
    -     d: number;
    -   } {
    +     d: number;}
    +   {
          fn(
          a,
          b,
          c,
    -     d
    -     );
    +     d);
    + 
          return {
            a,
            b,
            c,
    -       d
    -     };
    -   }
    - }
    +       d };
    + 
    +   }}

Note how this makes it look as if there are deliberate newlines in various places where there wasn't in the input code.

@benjie
Copy link
Author

benjie commented Sep 4, 2019

So unsurprisingly with the newlines being added a lot of source maps had to be updated. I wonder if withSource was not the right thing to use...? Anyway, I've manually gone through and updated a load of them but if there's a way I can do this without making the changes to the source maps please let me know.

@benjie
Copy link
Author

benjie commented Sep 4, 2019

To be clear: I've updated the source maps to make the tests pass, but I'm not sure if they're correct or not (or even how to check them).

@babel-bot
Copy link
Collaborator

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

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Sep 4, 2019

(Unrelated to the possible approval of this PR)

If your goal is to keep the original formatting as much as possible, I suggest to use recast.
Using recast with Babel is tricky, but you can find an example at https://github.com/nicolo-ribaudo/legacy-decorators-migration-utility/blob/1c9f306b5287bb2d5d07a512d1ad84036fd4956a/packages/wrap-legacy-decorators/src/index.js#L105

@nicolo-ribaudo nicolo-ribaudo added pkg: generator PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Sep 4, 2019
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: generator PR: Bug Fix 🐛 A type of pull request used for our changelog categories

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants