Simplify Settings by Relying on Users Setting Prettier Config

[robwise]

Resolves #395 (plugins will be free to run since settings are no inferred from the prettier config)
Resolves #401 (prose option will just be inferred from prettier config instead of needing to be set in prettier-atom)
Resolves #398 (prettier should just work for experimental features since prettier-atom no longer needs to duplicate new experimental options)
Resolves #178 (prettier-atom doesn't pass any options so there's no longer a need to clarify this)
Resolves #433 (prettier-atom will no longer rely on scopes to determine whether to format on save, it will ask Prettier to figure it out)
Resolves #406 (prettier-atom will now check Prettier version using a slightly different method so this logic bug is no longer a problem)

[codecov-io]

Codecov Report

Merging #404 into master will decrease coverage by 5.14%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #404      +/-   ##
==========================================
- Coverage   78.61%   73.46%   -5.15%     
==========================================
  Files          28       29       +1     
  Lines         505      456      -49     
  Branches       56       46      -10     
==========================================
- Hits          397      335      -62     
- Misses         92      103      +11     
- Partials       16       18       +2

Impacted Files	Coverage Δ
src/helpers/getPrettierPath.js	100% <ø> (ø)	⬆️
src/helpers/index.js	100% <100%> (ø)	⬆️
src/manualFormat/index.js	100% <100%> (ø)	⬆️
src/executePrettier/handleError.js	100% <100%> (ø)	⬆️
src/helpers/isPrettierProperVersion.js	100% <100%> (ø)
src/formatOnSave/shouldFormatOnSave.js	100% <100%> (ø)	⬆️
src/editorInterface/index.js	73.68% <100%> (-6.32%)	⬇️
src/helpers/isFileFormattable.js	100% <100%> (ø)
src/statusTile/updateStatusTileScope.js	100% <100%> (ø)	⬆️
src/atomInterface/index.js	91.89% <100%> (+3.65%)	⬆️
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6587f2...d6a25f5. Read the comment docs.

[robwise]

I still need to do some manual Q/A as well as check out whether we can get rid of the new scopes option I added (which simply determines whether or not prettier-atom should run based on the current Atom scope, as opposed to the multiple scope options we had previously that did this as well as chose which parser to use. Now we just let prettier decide based on the filepath).

[robwise]

Waiting on prettier/prettier#4253 to be implemented as we need prettier to expose whether or not the file is parseable or not if we're not going to use prettier-atom settings to do this.

[robwise]

Since this is going to break all builds that do not use the very latest prettier, we might as well refactor to use this as well: prettier/prettier#3846

[kachkaev]

@robwise WDYT of using Prettier via CLI rather than through require()? This way Atom would resolve and use the most up-to-date instance at all times. Otherwise, upgrading Prettier or, say, installing a global instance with a couple of plugins does not change the way formatting works until Atom is relaunched. I guess this is to do with how required modules are cached by Electron and it would be great if this side-effect was avoided. CLI approach should also make it easier to discover the global instance because the extension can simply try exec('prettier', [...]) rather than hand-picking it from global npm or yarn modules.

[robwise]

@robwise WDYT of using Prettier via CLI rather than through require()? This way Atom would resolve and use the most up-to-date instance at all times...I guess this is to do with how required modules are cached by Electron

@kachkaev
IIRC, we're caching this ourselves in the package on purpose to reduce time to format, it's not an Electron thing. You'll notice the first time you format after starting is pretty slow, that's partly because we're looking for prettier for the first time and haven't primed the cache.

This is pretty standard fare for Atom packages though and I believe the same behavior (needing to restart Atom to pick up a change in local version of a module/package/gem/bin) is observed for things like linter-eslint, linter-flow, linter-rubocop, etc.

We also had a contributor test CLI vs. require and CLI was significantly slower for some reason.

So I'm sort of hesitant to change it ATM.

[kachkaev]

we're caching this ourselves in the package on purpose to reduce time to format, it's not an Electron thing

Does this mean that we can cache multiple projects' Prettier in paraller? That seems a pretty important thing to have.

Imagine I have a single Atom window with three projects: one using local Prettier 1.12.1 with no plugins, another using Prettier 1.13.0 with a couple of local plugins and a third one falling back to a global Prettier, which has a globally installed plugin (different from project 2). I can imagine how this can work when Prettier is called via CLI, but how about JS API?

[robwise]

@kachkaev We're using findCached from atom-linter to do this. You'd have to dive into the internals of that, but I believe It's all filename-based so the Atom project shouldn't be relevant AFAIK.

[robwise]

@kachkaev This should be working now. Do you want to test it for a bit (I will do the same), especially with Prettier plugins enabled?

[kachkaev]

Many thanks @robwise!

I’m on holidays now, so won’t be able to test things in the next week. But I trust your experience!

[robwise]

Does everyone think it would be okay to release this finally? I'm super scared because this might break a lot of projects for a lot of people.

[kachkaev]

I guess this should happen sooner or later anyway given the direction into which Prettier goes. If you anticipate a lot of broken workflows, how about adding one paragraph to README that would suggest how to temporary roll back?

This command should be a remedy while people migrate from eslint integration etc.:

apm install [email protected]

[robwise]

When Prettier is up-to-date, editing a malformed .elm file still triggers prettier-atom failed: undefined. Formatting a malformed .js file fails on save silently with no feedback from Prettier.

@kachkaev I can't reproduce this one. If it's a normal syntax error from Prettier (as opposed to one of these weird "undefined" errors it throws which causes a full on popup), we display it via the linter package (or nuclide if you're using that instead). Maybe you aren't noticing the error in the linter? It should show up as a red underline.

[robwise]

Okay I changed the message to not mention the local package specifically since this isn't always where it's pulling from, now it's just generic.

[kachkaev]

I see a change in the error message, thank you. Although it can be still improved by only mentioning the local prettier or the global one, I think it's good enough already!

It's strange that you can't reproduce prettier-atom failed: undefined error message as I'm getting it all the time. Here is how you may get to the error:

1. npm i -g prettier prettier-plugin-elm
   cd /tmp
   mkdir test-project
   cd test-project
   
   cat <<EOF > broken.js
   output "hello world"
   EOF
   
   cat <<EOF > working.js
   output = "hello world"
   EOF
   
   
   cat <<EOF > Broken.elm
   output "hello world"
   EOF
   
   cat <<EOF > Working.elm
   output = "hello world"
   EOF
   
   atom -d .

2. Cmd+save in all files

   • broken.js - nothing changes (built-in linter highlights the row)

   • working.js - semicolon added

   • Broken.elm - error shows up: prettier-atom failed: undefined

   • Working.elm - morphs to:

     module Main exposing (..)
     
     
     output =
         "hello world"

The behaviour does not depend on the presense of Elm plugin in Atom. The same happens with @prettier/plugin-php and *.php files.

[robwise]

It's strange that you can't reproduce prettier-atom failed: undefined error message

Oh I was saying I can't reproduce where you don't get any feedback when formatting a bad JS file as I do still get a linter error. I think with the other languages like elm or php, the plugins or whatever is parsing is not throwing the error that Prettier normally expects so it just calls it undefined?

[kachkaev]

I don’t believe that prettier interacts with liters unless this is happening inside the atom plugin (something like ‘linting report is not empty -> suppress exceptions’). So unless this is the case, the reason for this undefined popup can be in that plugins throw Error instead of SyntaxError.

Can you suppress that popup someone (at least for the time being)? I a real scenario, users of prettier plugins will also have linters installed for those extra languages. So when no formatting on save happens, something will be highlighted in Elm and PHP by that other plugin.

In my test case I did not have any additional linters installed as this was a blank Atom setup.

[robwise]

So unless this is the case, the reason for this undefined popup can be in that plugins throw Error instead of SyntaxError.

Yes, sorry, this is the same thing as what I was attempting to say. I think we're on the same page here.

Can you suppress that popup someone (at least for the time being)? I a real scenario, users of prettier plugins will also have linters installed for those extra languages. So when no formatting on save happens, something will be highlighted in Elm and PHP by that other plugin.

Hmm, perhaps we can have a special option as sort of a band-aid until plugins are better supporting errors? Some ideas:

• "Don't show errors when formatting": probably too extreme
• "Don't show undefined errors when formatting": maybe this is what we want, although it might be a bit hard to understand if you aren't familiar with the issue at hand.
• "Suppress errors for scopes": then you provide a list of Atom scopes where you want to suppress errors
• "Suppress undefined errors for scopes": same as above but only suppresses the undefined error

[kachkaev]

I agree with you that some options look a bit hard to explain. What if we simply suppress undefined errors without any options and console.log them in Atom’s developer tools? This will make prettier’s behavior look the same for any language at start and we can think of better UX after getting feedback from people.

I don’t exclude that this false positive error message originates from the plugins and has to be fixed upstream. However, given that it may take too much time and the current behavior may feel frustrating, it feels like implementing a special case for undefined is the least evil.

No matter how hard we try to cover edge cases and make the plugin ideal, I’m sure there will be scenarios that we’ve missed. The redesign you were working on is already a breakthrough and I feel really keen to see it on the battlefield! The faster this happens, the quicker the community will polish things up. Iteration over perfection ;–)

[robwise]

Yes, I agree on all points! I will implement this (suppress undefined errors and console.log them) and then I think we're ready for release!

[robwise]

Okay all set, ready to ship finally?

[kachkaev]

I believe so! 🕺

[kachkaev]

[billyjanitsch]

Hi! I authored one of the issues that this PR is meant to close (#401).

I understand why you'd want to simplify this plugin to reduce the maintenance burden, but FWIW, I'm a bit sad to see this merged. As I mentioned in #401 (comment), my primary use case for this plugin is to format one-off files that are not part of a project and therefore don't have a Prettier config (e.g. individual markdown documents, or small gists). If I understand this PR correctly, this is no longer possible?

[kachkaev]

@billyjanitsch this change should allow your use case. Just as before, prettier-atom tries a project-scoped instance of prettier, then falls back to the global prettier and finally uses a built-in prettier if none exist.

If you want some config to be applied outside your project, simply create .prettierconfig in a place like your home folder or even disk root dir – and it will be picked up. The plugin is no longer responsible for spoon-feeding options into prettier, but it does not mean you cannot configure it!

[robwise]

Please let me know if the flow @kachkaev described does not work for you, as I would consider that a bug and will fix ASAP.