Navigation Menu

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.

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

Merged
merged 11 commits into from Nov 24, 2022
Merged

Conversation

broofa
Copy link
Contributor

@broofa broofa commented Nov 13, 2022

This [hopefully] fixes #185 by extracting Directive nodes before processing the AST, and then re-injecting them as part of the file() 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.

Copy link

@oliveiraantoniocc oliveiraantoniocc left a 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.

@oliveiraantoniocc
Copy link

@ayusharma could you please approve this?

@broofa
Copy link
Contributor Author

broofa commented Nov 18, 2022

@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.

@vtv6
Copy link

vtv6 commented Nov 20, 2022

any update on this pull request?

Copy link
Collaborator

@byara byara left a 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?

Copy link
Collaborator

@byara byara left a 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!

@broofa
Copy link
Contributor Author

broofa commented Nov 21, 2022

@byara: Just pinging you to note my last change that refactors the traverse() logic that I'd copy/pasta'd from preprocessor into the get-code-from-ast test into it's own module/method. (i.e. so it's no longer copy/pasted.)

Copy link
Collaborator

@ayusharma ayusharma left a 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

@ayusharma
Copy link
Collaborator

@byara could you please release this?

@transitive-bullshit
Copy link
Contributor

transitive-bullshit commented Nov 24, 2022

Amazing job @broofa 🙏

As a workaround until this is released, you can use:

pnpm add @trivago/prettier-plugin-sort-imports@npm:@fisch0920/prettier-plugin-sort-imports

(or the equivalent for your package manager)

@fisch0920/prettier-plugin-sort-imports is just @broofa's fork built and published to npm.

@byara byara merged commit 0dad01d into trivago:master Nov 24, 2022
@byara
Copy link
Collaborator

byara commented Nov 24, 2022

Released in v4.0.0

FYI @broofa

@simon-dk
Copy link

Released in v4.0.0

Wow, that was fast! Much appreciated, and good job @broofa 👍

@transitive-bullshit
Copy link
Contributor

Thanks as well @byara 🙏 😄

@Keshavdulal
Copy link

Is this the default behaviour? @broofa
I couldn't find anything on docs for Directives.

How do we achieve this?
I am on 4.1.1 and trying to add "use client" directive on a nextjs project, but it keeps going down below imports as

;('use client')

This is my .prettierrc

{
  "singleQuote": true,
  "jsxSingleQuote": true,
  "arrowParens": "avoid",
  "bracketSpacing": true,
  "printWidth": 100,
  "useTabs": false,
  "tabWidth": 2,
  "semi": false,
  "trailingComma": "es5",
  "jsxBracketSameLine": true,
  "endOfLine": "auto",
  "quoteProps": "as-needed",
  "importOrder": ["^@core/(.*)$", "^@server/(.*)$", "^@ui/(.*)$", "^[./]"],
  "importOrderSeparation": true,
  "importOrderSortSpecifiers": true
}

Thanks.

@cglacet
Copy link

cglacet commented Oct 9, 2023

@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 ('use client'); and not ;('use client'))

@askjervold
Copy link

I get the same as @cglacet with version 4.3.0. I ended up switching to use eslint-plugin-simple-import-sort to handle this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
10 participants