Fix handling of comments with decorators before `export` #15032

nicolo-ribaudo · 2022-10-10T15:51:40Z

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

babel-bot · 2022-10-10T15:54:45Z

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

JLHwung · 2022-10-17T20:45:29Z

packages/babel-parser/src/parser/statement.ts

+      // comments between `export` and `class` as leading comments of the
+      // class declaration node.
+      // This is needed because the class location range overlaps with
+      // `export`, and thus they would me marked as inner comments of


Suggested change

// `export`, and thus they would me marked as inner comments of

// `export`, and thus they would be marked as inner comments of

JLHwung · 2022-10-17T20:45:56Z

packages/babel-parser/src/parser/statement.ts

+      // `export`, and thus they would me marked as inner comments of
+      // the class declaration.
+      //   @dec export /* comment */ class Foo {}
+      // See [parseDecorators] for the hanling of comments before decorators


Suggested change

// See [parseDecorators] for the hanling of comments before decorators

// See [parseDecorators] for the handling of comments before decorators

JLHwung · 2022-10-17T20:53:04Z

packages/babel-parser/src/parser/statement.ts

+          lastTokEndLoc: { index: lastTokEnd },
+        } = this.state;
+
+        if (this.input.slice(lastTokStart, lastTokEnd) === "export") {


Can we move the comments manipulation to

babel/packages/babel-parser/src/parser/comments.ts

Line 164 in ddaee90

finalizeComment(commentWS: CommentWhitespace) {

?

So that comments manipulation are placed within a single file and we probably don't have to do an export lookback.

I moved the logic to comments.ts, but I'm not sure about how to do it in finalizeComment. Specifically, I don't think we can distinguish /* 1 */ and /* 2 */ when finalizing the comments of the class node:

@dec export default /* 1 */ class /* 2 */ extends X {}

(I just realized that this PR doesn't support the default case, I'll add it)

.

nicolo-ribaudo · 2022-10-25T13:05:15Z

I completely rewrote how decorators are attached to classes:

first we finalize the ClassDeclaration/ExportNamedDeclaration nodes, attaching their comments
then we attach the decorators

By doing so we don't need any custom comment adjustment logic. While doing so I also removed the decoratorStack state, which was not needed anymore.

nicolo-ribaudo · 2022-10-25T13:18:53Z

There are still some generator bugs

nicolo-ribaudo · 2022-10-25T15:27:16Z

Before this PR:

@dec export class A {}
    ^^^^^^^^^^^^^^^^^^
           ExportDeclaration

^^^^^^^^^^^^^^^^^^^^^^
   ClassDeclaration

^^^^
Decorator

After this PR:

@dec export class A {}
^^^^^^^^^^^^^^^^^^^^^^
   ExportDeclaration

^^^^^^^^^^^^^^^^^^^^^^
   ClassDeclaration

^^^^
Decorator

(in both cased the nodes hierarchy if ExportDeclaration > ClassDeclaration > Decorator)

The "unusual" comment attachment only happens when there is @dec export class. In all the other cases comments follow the common nesting rules, so

in /* 1 */ @d /* 2 */ class A {}, 1 is a leading comment of the class and 2 a trailing comment of the decorator
in /* 1 */ @d export /* 2 */ class A {} 1 is a leading comment of the export declaration and 2 a leading comment of the class.

liuxingbaoyu

Great job!
Also the E2E test failure is related, I'm not sure if that is expected.

packages/babel-parser/src/parser/statement.ts

Co-authored-by: liuxingbaoyu <[email protected]>

nicolo-ribaudo · 2022-10-25T17:13:20Z

@fisker / @sosukesuzuki How does prettier decide when to print decorators before export and when to print them after?

JLHwung · 2022-10-25T17:23:19Z

I believe e2e test is breaking because prettier uses location of first decorator versus the attached class node to detect decorators before export:

https://github.com/prettier/prettier/blob/ca246afacee8e6d5db508dae01730c9523bbff1d/src/language-js/print/decorators.js#L76

nicolo-ribaudo · 2022-10-25T17:25:45Z

Uhm ok. I can restore the original location of the export declaration, but I believe it's wrong and in Babel 8 we should make it include decorators again (because the a node should always start before is children and end after).

If prettier uses tokens when printing, they can check if the export token comes before or after the first decorator.

liuxingbaoyu · 2022-10-25T17:30:24Z

prettier/prettier#12806 (comment)
They should not use tokens.

JLHwung

I agree that the new location setting makes sense to me. Since prettier has pinned @babel/parser so it should be fine even if we ship as-is. Can prettier support the new location setting here? If so we don't have to defer to Babel 8.

nicolo-ribaudo · 2022-10-25T17:40:58Z

Another option that prettier has is to check if inputSource[exportDeclarationNode.start] === "@". Prettier maintainers, how does that sound?

fisker · 2022-10-25T17:45:20Z

New location make sense, don't need worry about us, we'll tweak logic.

fisker · 2022-10-26T04:18:51Z

End up with locStart(node) === locStart(decorators[0]) solution, https://github.com/prettier/prettier/pull/13733/files

nicolo-ribaudo added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: parser area: comments Spec: Decorators Spec: Decorators (Legacy) labels Oct 10, 2022

nicolo-ribaudo force-pushed the decoratorsBeforeExport-comments branch from 792f629 to 4acb0d3 Compare October 12, 2022 19:55

liuxingbaoyu previously approved these changes Oct 13, 2022

View reviewed changes

JLHwung reviewed Oct 17, 2022

View reviewed changes

nicolo-ribaudo force-pushed the decoratorsBeforeExport-comments branch from c28374d to 0a7c85d Compare October 25, 2022 13:03

nicolo-ribaudo marked this pull request as draft October 25, 2022 13:18

nicolo-ribaudo added 8 commits October 25, 2022 17:27

Fix handling of comments with decorators before export

232f242

decoratorsBeforeExport removed in babel 8

aece548

Move to comments.ts

7094d0d

Refactor decorators parsing, attach decorators after comments

599fa75

Add generator tests

1c1fa44

Simplify

6d0bc46

Fix range of export declaration

3c01491

Print inner comments in export default

4007d8c

nicolo-ribaudo force-pushed the decoratorsBeforeExport-comments branch from 592e9d0 to 4007d8c Compare October 25, 2022 15:30

Final fix

8f1ba1d

nicolo-ribaudo marked this pull request as ready for review October 25, 2022 15:42

nicolo-ribaudo requested review from liuxingbaoyu and JLHwung October 25, 2022 15:42

liuxingbaoyu approved these changes Oct 25, 2022

View reviewed changes

packages/babel-parser/src/parser/statement.ts Outdated Show resolved Hide resolved

packages/babel-parser/src/parser/statement.ts Outdated Show resolved Hide resolved

Update packages/babel-parser/src/parser/statement.ts

775781d

Co-authored-by: liuxingbaoyu <[email protected]>

liuxingbaoyu added the pkg: generator label Oct 25, 2022

JLHwung approved these changes Oct 25, 2022

View reviewed changes

liuxingbaoyu added PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release and removed PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release labels Oct 25, 2022

nicolo-ribaudo merged commit 362f15b into babel:main Oct 25, 2022

nicolo-ribaudo deleted the decoratorsBeforeExport-comments branch October 25, 2022 18:18

fisker mentioned this pull request Oct 26, 2022

Update babel to v7.20.0 prettier/prettier#13733

Merged

4 tasks

This was referenced Oct 31, 2022

[Bug]: Some issue in @babel/parser version 7.20.0 #15085

Closed

Fix(estree): Reset export's start after decorator #15107

Merged

shaneomatz mentioned this pull request Nov 18, 2022

[Snyk] Upgrade @babel/runtime from 7.12.5 to 7.20.0 shaneomatz/video.js#1

Open

linshuohao mentioned this pull request Dec 19, 2022

fix(compiler-sfc): fix rewriteDefault problem when using @babel/parser@^7.20.0 vuejs/vue#12906

Closed

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jan 26, 2023

github-actions bot locked as resolved and limited conversation to collaborators Jan 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix handling of comments with decorators before `export` #15032

Fix handling of comments with decorators before `export` #15032

nicolo-ribaudo commented Oct 10, 2022 •

edited by gitpod-io bot

Loading

babel-bot commented Oct 10, 2022 •

edited

Loading

JLHwung Oct 17, 2022

JLHwung Oct 17, 2022

JLHwung Oct 17, 2022

nicolo-ribaudo Oct 24, 2022

nicolo-ribaudo commented Oct 25, 2022 •

edited

Loading

nicolo-ribaudo commented Oct 25, 2022

nicolo-ribaudo commented Oct 25, 2022 •

edited

Loading

liuxingbaoyu left a comment

nicolo-ribaudo commented Oct 25, 2022

JLHwung commented Oct 25, 2022 •

edited

Loading

nicolo-ribaudo commented Oct 25, 2022 •

edited

Loading

liuxingbaoyu commented Oct 25, 2022

JLHwung left a comment

nicolo-ribaudo commented Oct 25, 2022 •

edited

Loading

fisker commented Oct 25, 2022

fisker commented Oct 26, 2022

	// `export`, and thus they would me marked as inner comments of
	// `export`, and thus they would be marked as inner comments of

	// See [parseDecorators] for the hanling of comments before decorators
	// See [parseDecorators] for the handling of comments before decorators

Fix handling of comments with decorators before export #15032

Fix handling of comments with decorators before export #15032

Conversation

nicolo-ribaudo commented Oct 10, 2022 • edited by gitpod-io bot Loading

babel-bot commented Oct 10, 2022 • edited Loading

JLHwung Oct 17, 2022

Choose a reason for hiding this comment

JLHwung Oct 17, 2022

Choose a reason for hiding this comment

JLHwung Oct 17, 2022

Choose a reason for hiding this comment

nicolo-ribaudo Oct 24, 2022

Choose a reason for hiding this comment

nicolo-ribaudo commented Oct 25, 2022 • edited Loading

nicolo-ribaudo commented Oct 25, 2022

nicolo-ribaudo commented Oct 25, 2022 • edited Loading

liuxingbaoyu left a comment

Choose a reason for hiding this comment

nicolo-ribaudo commented Oct 25, 2022

JLHwung commented Oct 25, 2022 • edited Loading

nicolo-ribaudo commented Oct 25, 2022 • edited Loading

liuxingbaoyu commented Oct 25, 2022

JLHwung left a comment

Choose a reason for hiding this comment

nicolo-ribaudo commented Oct 25, 2022 • edited Loading

fisker commented Oct 25, 2022

fisker commented Oct 26, 2022

Fix handling of comments with decorators before `export` #15032

Fix handling of comments with decorators before `export` #15032

nicolo-ribaudo commented Oct 10, 2022 •

edited by gitpod-io bot

Loading

babel-bot commented Oct 10, 2022 •

edited

Loading

nicolo-ribaudo commented Oct 25, 2022 •

edited

Loading

nicolo-ribaudo commented Oct 25, 2022 •

edited

Loading

JLHwung commented Oct 25, 2022 •

edited

Loading

nicolo-ribaudo commented Oct 25, 2022 •

edited

Loading

nicolo-ribaudo commented Oct 25, 2022 •

edited

Loading