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

Opt-in to use included prettier version #248

Closed
sQVe opened this issue Sep 10, 2021 · 7 comments · Fixed by #250
Closed

Opt-in to use included prettier version #248

sQVe opened this issue Sep 10, 2021 · 7 comments · Fixed by #250

Comments

@sQVe
Copy link
Contributor

sQVe commented Sep 10, 2021

Hey 👋🏼,

If prettierd doesn't find a local version is falls back to the included version in this package. This is an issue in cases where a project doesn't run prettier and you introduce a lot of changes, which mainly is due to formatting.

I would argue that this is unwanted behavior and should not be the default but rather an opt-in option. What are your thoughts?

This is the only reason I'm currently using prettier_d_slim instead of this project, but since my PR (mikew/prettier_d_slim#11) never get merged I'd like to move over to this project and help out when I can.

@fsouza
Copy link
Owner

fsouza commented Sep 10, 2021

I think that's a fair ask. Would you be ok with it being opt-out? I'm a bit worried with changing the default behavior and breaking other people's workflow.

@sQVe
Copy link
Contributor Author

sQVe commented Sep 10, 2021

Yeah. While not optimal I understand the wish of not breaking an already established default.

Give me a ping if you need help with the implementation!

@sQVe
Copy link
Contributor Author

sQVe commented Sep 10, 2021

This is also a great feature in situations where the bundled prettier version is fine for some file types.

I would want to use the bundled version for json or markdown but not JavaScript and Typescript.

@fsouza
Copy link
Owner

fsouza commented Sep 11, 2021

@sQVe yeah just wondering what's the best way to configure prettierd. We can keep adding environment variables, but I imagine that at some point we'll want to introduce a config file in some location ($XDG_CONFIG_HOME or something like that).

Not sure if you have any opinions on that.

@sQVe
Copy link
Contributor Author

sQVe commented Sep 11, 2021

I would actually prefer to configure it via the CLI. But then again I'm not interested in providing a global prettier config - I would fallback to the default in those cases.

I would say that we optimally should create a prettierd dot file. Options via CLI overrides already set options. That would probably cover all use-cases.

fsouza added a commit that referenced this issue Sep 11, 2021
The idea is that when PRETTIERD_DISABLE_BUNDLED_PRETTIER is set and
prettier isn't installed locally, we want to skip formatting and just
return the file as is.

Closes #248.
@fsouza
Copy link
Owner

fsouza commented Sep 11, 2021

@sQVe opened #250 so you can have a look, we can also bikeshed on the name for the environment variable :)

Will put off introducing a config file or flags for now, and will revisit if/when we get too many environment variables.

fsouza added a commit that referenced this issue Sep 11, 2021
The idea is that when PRETTIERD_DISABLE_BUNDLED_PRETTIER is set and
prettier isn't installed locally, we want to skip formatting and just
return the file as is.

Closes #248.
fsouza added a commit that referenced this issue Sep 11, 2021
The idea is that when PRETTIERD_DISABLE_BUNDLED_PRETTIER is set and
prettier isn't installed locally, we want to skip formatting and just
return the file as is.

Closes #248.
@sQVe
Copy link
Contributor Author

sQVe commented Sep 11, 2021

Closing this as #250 is a great first solution for this problem.

@sQVe sQVe closed this as completed Sep 11, 2021
fsouza added a commit that referenced this issue Sep 11, 2021
The idea is that when PRETTIERD_DISABLE_BUNDLED_PRETTIER is set and
prettier isn't installed locally, we want to skip formatting and just
return the file as is.

Closes #248.
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 a pull request may close this issue.

2 participants