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

Fix prettier importOrderSeparation not working #1796

Merged
merged 5 commits into from Feb 11, 2022
Merged

Fix prettier importOrderSeparation not working #1796

merged 5 commits into from Feb 11, 2022

Conversation

denik1981
Copy link
Contributor

What does this PR do?

Prettier's configuration file is wrongly composed adding the Tailwind Plugin in a declarative way within the plugin key. Prettier mentions that third-party plugins are automatically loaded if they have been installed in the same node_modules folder and follow the name of @prettier/<plugin> or prettier-plugin-<plugin> (prettier-on-how-to-add-third-party-plugins)

Placing a plugin key within prettierrc.js causes a not expected behavior because despite prettier actually runs, it fails on applying certain rules, being importOrderSeparation one of them. A clear side effect for this is within issue #1791 where a missing space between the first import in all the prisma/zod/* files is causing all of them to differ from HEAD when they are regenerated by prisma (and later formatted by prettier)

Fixes # (1791)

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How should this be tested?

Test A

Check that prisma/zod/* files don't differ from HEAD after regenerating them.

  • yarn generate-schemas
  • git diff --name-only | grep "prisma/zod" || echo "PASSED"

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code and corrected any misspellings
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@vercel
Copy link

vercel bot commented Feb 10, 2022

@denik1981 is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@denik1981
Copy link
Contributor Author

I believe this PR is still relevant despite the new turbo addition into the stack, but please put this on pause until I ran a second test with the new structure of the repo.

@zomars
Copy link
Member

zomars commented Feb 11, 2022

The problem is that If we remove the plugin declaration, the tailwind classes aren't sorted. You can try yourself locally.

@denik1981
Copy link
Contributor Author

The problem is actually an incompatibility issue between prettier-plugin-tailwindcss and @trivago/prettier-plugin-sort-imports . Seem both plugins cannot be used together nicely. This issue has been mentioned in both repos.

tailwindlabs/prettier-plugin-tailwindcss#9
tailwindlabs/prettier-plugin-tailwindcss#31
trivago/prettier-plugin-sort-imports#117

Leaving prettierrc as it is will make having the sorted imports plugin useless. I have studied the workaround of the chained parser but I don't think it will work well with the IDE autosave or even with eslint. imho the only way to tackle this right now is to remove one of the two to prevent inconsistencies or work in a fork in the tw repo to make both of these plugins compatible.

@denik1981
Copy link
Contributor Author

denik1981 commented Feb 11, 2022

btw, regarding #1791, why it is necessary to vc prisma/zod files if they are fully generated in the post installation step by prisma? Can we gitignore them?

@PeerRich
Copy link
Member

tailwindlabs/prettier-plugin-tailwindcss#31 (comment) this seems to at least hotfix it

@zomars
Copy link
Member

zomars commented Feb 11, 2022

btw, regarding #1791, why it is necessary to vc prisma/zod files if they are fully generated in the post installation step by prisma. Can we gitignore them?

Fair point @denik1981 we would probably just prettier ignore those files

@denik1981
Copy link
Contributor Author

Ok, I will try it to see how it works and push it today.

@zomars zomars added the ♻️ autoupdate tells kodiak to keep this branch up-to-date label Feb 11, 2022
@vercel
Copy link

vercel bot commented Feb 11, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

docs – ./apps/docs

🔍 Inspect: https://vercel.com/cal/docs/5fNu4jpuTenvB1zBZNrsGrLn4566
✅ Preview: https://docs-git-fork-denik1981-fix-prettier-not-responsive-3d2aac-cal.vercel.app

[Deployment for de5416c canceled]

calendso – ./apps/web

🔍 Inspect: https://vercel.com/cal/calendso/qZnBS5dyezQmVkhYNu3fDTcFw6zr
✅ Preview: Canceled

@vercel vercel bot temporarily deployed to Preview – docs February 11, 2022 20:15 Inactive
@vercel vercel bot temporarily deployed to Preview – calendso February 11, 2022 20:16 Inactive
@zomars
Copy link
Member

zomars commented Feb 11, 2022

The linter is working "too well" now 🤣 will merge as it is for now and we can fix those error on a followup.
image

@denik1981
Copy link
Contributor Author

@zomars Thanks!
I can confirm that the hotfix works well for autosave, linting and formatting.

This (below) is an emulation in an isolated nextjs app with all the Cal settings for prettier and eslint and tests added to it.
https://stackblitz.com/github/denik1981/prettier-tailwind-trivago-hotfix

npm run test:should-fail unformats a random file and then runs the linter
npm run test:should-pass formats a random file and then runs the linter.

The reason I delayed this PR is because the hotfix throw me an error that I was not able to reproduce anymore.
Not sure the reason cause it is trying to parse an MD file but I will post it here just in case appears again.

> yarn  format
yarn run v1.22.17
$ prettier --write "**/*.{ts,tsx,md}"
[error] Invalid configuration file `.github/ISSUE_TEMPLATE/bug_report.md`: Obejct is not defined

@PeerRich
Copy link
Member

@zomars Thanks! I can confirm that the hotfix works well for autosave, linting and formatting.

This (below) is an emulation in an isolated nextjs app with all the Cal settings for prettier and eslint and tests added to it. https://stackblitz.com/github/denik1981/prettier-tailwind-trivago-hotfix

npm run test:should-fail unformats a random file and then runs the linter npm run test:should-pass formats a random file and then runs the linter.

The reason I delayed this PR is because the hotfix throw me an error that I was not able to reproduce anymore. Not sure the reason cause it is trying to parse an MD file but I will post it here just in case appears again.

> yarn  format
yarn run v1.22.17
$ prettier --write "**/*.{ts,tsx,md}"
[error] Invalid configuration file `.github/ISSUE_TEMPLATE/bug_report.md`: Obejct is not defined

oh whats the best way moving forward? @zomars

@zomars
Copy link
Member

zomars commented Feb 11, 2022

That's odd. I can't seem to replicate locally. Will investigate further.

zomars added a commit that referenced this pull request Feb 15, 2022
* Fix prettier importOrderSeparation not working

* Solves prettier plugin conflict

Co-authored-by: Peer Richelsen <peeroke@gmail.com>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Co-authored-by: zomars <zomars@me.com>
buschco pushed a commit to buschco/calendso that referenced this pull request Mar 27, 2022
* Fix prettier importOrderSeparation not working

* Solves prettier plugin conflict

Co-authored-by: Peer Richelsen <peeroke@gmail.com>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Co-authored-by: zomars <zomars@me.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge ♻️ autoupdate tells kodiak to keep this branch up-to-date
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants