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 v3 and up resolveConfig broken #97

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

wikes82
Copy link

@wikes82 wikes82 commented Apr 10, 2024

Fix for issue #94

@wikes82 wikes82 requested a review from valtyr as a code owner April 10, 2024 15:24
@valtyr
Copy link
Owner

valtyr commented May 8, 2024

Hey, sorry for the late reply. Are you sure this doesn't break prettier configs that are specified in package.json files for example?

@valtyr
Copy link
Owner

valtyr commented May 8, 2024

Okay interesting. It seems like the resolveConfig method wants the path to the source file in question. Maybe we can pass in the destination path for the generated file here instead of .prettierrc?

https://github.com/prettier/prettier/blob/main/docs/api.md#prettierresolveconfigfileurlorpath--options

@wikes82
Copy link
Author

wikes82 commented May 8, 2024

Hey, sorry for the late reply. Are you sure this doesn't break prettier configs that are specified in package.json files for example?

how about just calling resolveConfig() without parameter ? I tested on my end and it works.

@wikes82
Copy link
Author

wikes82 commented May 8, 2024

Okay interesting. It seems like the resolveConfig method wants the path to the source file in question. Maybe we can pass in the destination path for the generated file here instead of .prettierrc?

https://github.com/prettier/prettier/blob/main/docs/api.md#prettierresolveconfigfileurlorpath--options

I don't understand this, don't you want to pass user's project prettier config file into resolveConfig ?

@valtyr
Copy link
Owner

valtyr commented May 21, 2024

@wikes82 If I understand correctly they want the path to the file that prettier is meant to be run on. That is the path to the .ts source file. If calling resolveConfig without a file works I guess that would be fine. It must search from the current working directory and up, but that might be ambiguous in some cases. What do you think?

@wikes82
Copy link
Author

wikes82 commented May 21, 2024

@wikes82 If I understand correctly they want the path to the file that prettier is meant to be run on. That is the path to the .ts source file. If calling resolveConfig without a file works I guess that would be fine. It must search from the current working directory and up, but that might be ambiguous in some cases. What do you think?

Yes, passing the generated files folder path works too.
Should we do a fallback ? call resolveConfig without param first, then if the config still null, we call with generated files folder path?
To pass in generated folder path, we will need to change writeFileSafely function to accept GeneratorOptions right? will you be ok with it?

@arthurfiorette
Copy link

Would be useful if we could skip prettier formatting altogether. This step is taking out 3-4s of our build pipeline.

Wdyt @valtyr?

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

Successfully merging this pull request may close these issues.

3 participants