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

Consider removing default parser / Skip processing unsupported file types #2884

Closed
azz opened this issue Sep 23, 2017 · 35 comments
Closed

Consider removing default parser / Skip processing unsupported file types #2884

azz opened this issue Sep 23, 2017 · 35 comments
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:needs discussion Issues needing discussion and a decision to be made before action can be taken type:enhancement A potential new feature to be added, or an improvement to how we print something
Milestone

Comments

@azz
Copy link
Member

azz commented Sep 23, 2017

Currently when

  • Prettier is invoked from the CLI, and
  • The file being formatted doesn't match any of the extensions supported, and
  • No --parser option is passed, and
  • No parser is in a prettier config file,

then it will fall back to the babylon parser and treat the file as JavaScript. This made sense when prettier only supported JS (and then TypeScript), but now that it supports vastly different languages perhaps it is time to challenge that default.

In #2882, we discovered this can have negative repercussions when used with --write **/*, modifying files inside of a .git directory.

If we were to change the default from JavaScript to a no-op, these kind of issues would go away, and we'd also have less issues reported along the lines of "My CSS doesn't parse", when they're accidentally using the babylon parser.

It is already possible to associate other extensions with a parser in .prettierrc.

{
  "overrides": [{
     "files": "*.mjs",
     "options": { "parser": "babylon" }
  }]
}

Thoughts @lydell @vjeux?

@azz azz added the status:needs discussion Issues needing discussion and a decision to be made before action can be taken label Sep 23, 2017
@lydell
Copy link
Member

lydell commented Sep 25, 2017

I like the idea! (It might be worth thinking about #2846 at the same time, or even re-thinking the CLI overall. It is quite messy. At the same time we shouldn't cause unnecessary backwards incompatibility. So it might also be better to not mix anything else into the thought process of this issue. I don't know.)

@ikatyang ikatyang added the type:enhancement A potential new feature to be added, or an improvement to how we print something label Oct 15, 2017
@j-f1 j-f1 added this to the Prettier 2.0 milestone Jan 23, 2018
@nickmccurdy
Copy link
Member

Couldn't this be done without a breaking change? Prettier's parsing of incompatible files generally errors anyway, I don't think a noop would hurt anyone's workflow as those files would already not be formatted properly by Prettier. I do like the idea of #2846 but it seems like it could be done after.

@lydell lydell changed the title Consider removing default parser Consider removing default parser / Skip processing unsupported file types Mar 22, 2018
@suchipi
Copy link
Member

suchipi commented Apr 5, 2018

I consider this behavior a bug, not a feature, and by that logic, I think we should make this change without waiting for 2.0.

@lydell
Copy link
Member

lydell commented May 1, 2018

I think it's really important that we figure out a non-breaking way that fixes 80% of the cases as soon as possible, because lots of users are confused when Prettier tries to format HTML files, which sometimes succeeds and sometimes doesn't, depending on if the HTML is valid JSX or not.

Could this work out, maybe?

If Prettier is told to format a file with an extension it does not recognize and the parser option has not been set (either in the CLI or in a config file), print a warning for that file (Skipping file with unknown extension .html: /home/lydell/foo/src/index.html).

We have to be really careful so we don't break editor integrations that rely on the CLI such as vim-prettier (ping @mitermayer) though.

I also think we can consider this a bug and not wait for 2.0.

@kachkaev
Copy link
Member

kachkaev commented May 1, 2018

@lydell could this be implemented as built-in an HTML plugin, which simply does not do anything? I.e. it'll parse the entire text into a syntax tree with a single node and simply print it back as is.

@lydell
Copy link
Member

lydell commented May 1, 2018

@kachkaev That's a really clever solution if my proposal doesn't work out 👍

@kachkaev
Copy link
Member

kachkaev commented May 1, 2018

Making .html a special case behaviour sounds a bit convoluted from the implementation perspective and raises questions like Can I also add .py or .php as other special cases when corresponding plugins are not loaded? Prettier attempts to apply babel parser to these now too, which makes it reasonable to suggest that if we say Skipping file with unknown extension .html, the same should apply to other unknown extensions. And that's v2.

A dummy plugin for HTML files will require minimal changes to the architecture of Prettier and will be a right step towards properly implementing it one day. It will also save people from the confusion you are mentioning.

@lydell
Copy link
Member

lydell commented May 1, 2018

I didn't mean that we should special-case HTML. This would be the warning message template: `Skipping file with unknown extension .${extension}: ${filepath}.`

@kachkaev
Copy link
Member

kachkaev commented May 1, 2018

Where would that message go? Stderr?

Will .py and .php files be skipped with the same message (unless corresponding plugins are installed)? If so, that sounds like a v2 behaviour to me, which is right, but breaking.

@lydell
Copy link
Member

lydell commented May 1, 2018

Stderr?

Most likely, yes.

Will .py and .php files be skipped with the same message (unless corresponding plugins are installed)?

That was my idea, yes.If so, that sounds like a v2 behaviour to me, which is right, but breaking.

If so, that sounds like a v2 behaviour to me, which is right, but breaking.

Why?

@kachkaev
Copy link
Member

kachkaev commented May 1, 2018

Why?

Because currently Prettier swallows files with all extensions and if a parser cannot be determined, it uses a fallback babel parser. That's not ideal, yes, but that's how things are at the moment. Downstream projects like editor extensions can be using this 'feature' in ways we don't know, so if we want to change the behaviour, it can be only done in Prettier 2.0 (which would allow for breaking changes). Because Prettier adheres semantic versioning and this gives certain promises to the community, there are limitations to what can be changed within a single major version (currently v1).

@lydell
Copy link
Member

lydell commented May 1, 2018

Downstream projects like editor extensions can be using this 'feature' in ways we don't know

So let's ask them.

if we want to change the behaviour, it can be only done in Prettier 2.0

Then we're doomed :) Or we have to make a really boring 2.0 with only this change.

@vjeux
Copy link
Contributor

vjeux commented May 1, 2018

Fwiw, prettier doesn’t really adhere to semantic versioning, at every release, including patch ones, the output of some code will change, so it’s a breaking change as you’ll need to run it through your codebase to get you unblocked.

@kachkaev
Copy link
Member

kachkaev commented May 1, 2018

@vjeux if so, I'm totally happy with getting rid of the default parser skip formatting unless inferParser() returned a string for a file name or --parser is explicitly given.

So let's ask them.

@lydell not sure there exists a list of all developers who are using Prettier as part of their tools 😅

@lydell
Copy link
Member

lydell commented May 1, 2018

not sure there exists a list of all developers who are using Prettier as part of their tools

Let's just ask the most popular ones.

The question is: If prettier file/with/unknown.extension printed a warning to stderr instead of trying to format file/with/unknown.extension as JS, would that cause problems for editor integrations?

@rcoedo
Copy link

rcoedo commented May 1, 2018

It wouldn't be a problem for prettier-emacs. The users could conditionally add specific --parser options for custom or unsupported extensions.

@undeadcat
Copy link

undeadcat commented May 2, 2018

Hi! I'm the developer who wrote most of the code for the WebStorm integration.
It would not be a problem for WebStorm, we use getSupportInfo to determine if the file is acceptable.
Thank you so much for giving the heads up! In the future, feel free to @ - mention me also.

@vjeux
Copy link
Contributor

vjeux commented May 2, 2018

@undeadcat thanks so much for building it, it’s been a highly requested from people using prettier!

@mitermayer
Copy link
Member

Hi @lydell,

This is not a problem for vim-prettier as we always include the --parser option. I am currently on the process of rewritting most of it for 1.0 release so changes like that should be ok since we will do a major release with other breaking changes.

cc @docwhat

@duailibe
Copy link
Member

duailibe commented May 3, 2018

FTR I wrote https://github.com/duailibe/atom-miniprettier and it doesn't try to infer a parser at all, defers that completely to Prettier. It's been working great so far! :)

With this behavior, I won't have to change anything since that's exactly the behavior I want 😄

@kachkaev
Copy link
Member

kachkaev commented May 3, 2018

@duailibe so will your plugin try to prettify any file extension using babel parser as a fallback now? We try to avoid this with @robwise in #4341 to make prettier-atom lighter (prettier/prettier-atom#404).

@duailibe
Copy link
Member

duailibe commented May 3, 2018

@kachkaev for now, yes. But since I don't use autosave, that's not an actual problem for me.

I'm just commenting on that thread right now. :)

@suchipi
Copy link
Member

suchipi commented May 5, 2018

I opened a PR that implements this change, I need some help implementing the error message that @lydell mentioned in #2884 (comment), but otherwise it is functional

@suchipi
Copy link
Member

suchipi commented May 6, 2018

In the same vein, @mitermayer @rocedo @undeadcat @madskristensen we are thinking about changing the JavaScript API to require passing either parser or filepath, otherwise the text will be passed through as-is.

> prettier.format("function bla      () {}")
'function bla      () {}'
> prettier.format("function bla      () {}", { filepath: "foo.js" })
'function bla() {}\n'
> prettier.format("function bla      () {}", { parser: "babylon" })
'function bla() {}\n'

This will affect prettier.format, prettier.formatWithCursor, and prettier.check.

Would this adversely affect you? Are you always specifying either parser or filepath?

Context: #4426 (comment)

@j-f1
Copy link
Member

j-f1 commented May 6, 2018

cc @rcoedo since your username wasn’t highlighted in the above comment.

@suchipi
Copy link
Member

suchipi commented May 6, 2018

Thanks @j-f1

@rcoedo
Copy link

rcoedo commented May 7, 2018

We don't use the JS API in the emacs package 👍

@undeadcat
Copy link

The WebStorm plugin always passes filepath.

@duailibe
Copy link
Member

duailibe commented May 7, 2018

cc @robwise

@robwise
Copy link

robwise commented May 7, 2018

prettier-atom does not always pass filepath (in cases where the user has chosen to manually format text in a draft document that has not yet been saved as a file). Currently, we specify the parser, but that's about to change with #4341 where we will rely on Prettier to tell us what parser to use.

@zimme
Copy link
Member

zimme commented May 7, 2018

In prettier-eslint we let prettier decide what parser to use, however consumers of prettier-eslint's API is free to provide parser if they want.

@suchipi
Copy link
Member

suchipi commented May 7, 2018

@robwise Do you specify the parser for unsaved draft documents also? You wouldn't be able to pass a filepath for those even after #4341, so I'm wondering how you do it now. Do you use the editor scope? Maybe prettier needs to support editor scope for parser inference in addition to filepath?

@robwise
Copy link

robwise commented May 13, 2018

@suchipi sorry for late response. Currently, we always specify a parser based on Atom scope, but after #4341 we just don't pass a parser or filepath at all if the file is not saved and prettier reverts to a JS parser and attempts to format with that.

I would stay away from prettier supporting editor scope, that would quickly turn into a nightmare to support. IMO it really should be the editor integration's responsibility if we wanted to go that route, but we're purposely deleting all of the code doing this in #4341 because that paradigm makes it basically impossible to support the new prettier plugins feature.

@duailibe
Copy link
Member

@suchipi I think we're safe to go forward with this

@suchipi
Copy link
Member

suchipi commented May 15, 2018

Ok, cool

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Aug 21, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:needs discussion Issues needing discussion and a decision to be made before action can be taken type:enhancement A potential new feature to be added, or an improvement to how we print something
Projects
None yet
Development

No branches or pull requests