-
Notifications
You must be signed in to change notification settings - Fork 139
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
keep script directives at top of file, fixes #185 #186
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's a comment in front of it, the comment will end up being moved to a new line above the directive, for RSC most probably this will be incompatible, so no comments explaining why "use client"; // some explanation about directive will break RSC
.
This is certainly not critical, an solution would be keeping comments in front for directives.
@ayusharma could you please approve this? |
@ayusharma Thoughts? I, and I suspect others here, would very much like to get this merged and published ASAP as I don't believe there's a viable workaround for this issue other than to disable this plugin. |
any update on this pull request? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @broofa 👏
The PR looks good 👍 Would it be possible to add those two unit-test cases I mentioned?
If so I think we can merge it soon. What do you think @ayusharma?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
Thank you for your contribution!
@byara: Just pinging you to note my last change that refactors the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the changes ❤️. Looks good to me
@byara could you please release this? |
Amazing job @broofa 🙏 As a workaround until this is released, you can use:
(or the equivalent for your package manager)
|
Thanks as well @byara 🙏 😄 |
Is this the default behaviour? @broofa How do we achieve this?
This is my .prettierrc
Thanks. |
@Keshavdulal Same issue here. With the following dependencies: {
"devDependencies": {
"eslint": "^8.5",
"prettier": "^3",
"@trivago/prettier-plugin-sort-imports": "~4.2.0",
"turbo": "^1.4.2",
"typescript": "^5.2.2"
},
} (Not exactly the same as I get |
I get the same as @cglacet with version |
This [hopefully] fixes #185 by extracting
Directive
nodes before processing the AST, and then re-injecting them as part of thefile()
generation used to create the import section of the script... which should preserve them at top-of-file.Includes a new test case. Compiles cleanly, all tests pass.