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

prettier: true, but no spaces inside braces: {Foo} #614

Closed
devinrhode2 opened this issue Oct 6, 2021 · 11 comments
Closed

prettier: true, but no spaces inside braces: {Foo} #614

devinrhode2 opened this issue Oct 6, 2021 · 11 comments

Comments

@devinrhode2
Copy link
Contributor

I have prettier: true in my xo config, but, I'm seeing this formatting:

import {Foo} from './foo'

This is fundamentally the "object-curly-spacing" style rule from eslint (or @typescript-eslint/object-curly-spacing).

I see this is set inside eslint-config-xo and eslint-config-xo-typescript, to ["error", "never"] - "never" meaning we never want spacing around Foo.

I confirmed this by setting prettier: false inside my xo config, and inside vscode, I see a warning regarding this style rule.
Screen Shot 2021-10-05 at 08 59 44 PM

As a sanity check, I did run prettier directly:

yarn prettier --write /full/path/to/file.ts

And it restores spaces around Foo, resulting in:

import { Foo } from './foo'

I tried running yarn prettier --write /full/path/to/file.ts with multiple different prettier versions, 2.2.1, 2.3.2, 2.4.1, all of them restore spaces around Foo.

@devinrhode2
Copy link
Contributor Author

I tried modifying the rules inside of my xo config, setting them to ['error', 'always'] - which in eslint world, would restore the spaces around Foo - but - this was causing chaos when saving code inside vscode. Creating double spaces, trading one formatting error for a different type of formatting error.

@devinrhode2
Copy link
Contributor Author

devinrhode2 commented Oct 6, 2021

Q: Does the vscode extension support codeActionsOnSave?

=====

My general thinking is that we should address this at an architectural level, so that any formatting inconsistencies are not possible. I think that prettier should run after eslint, and eslint should ignore all formatting issues.

However, I would like to avoid writing eslint fixed code and then telling prettier to read from disk. Passing an ast directly to prettier would be awesome, but would be quite the undertaking.

@devinrhode2
Copy link
Contributor Author

The biggest overhead with eslint-plugin-prettier I'm guessing is probably the generateDifferences call.

I will check what the prettier.format( call is returning today

@devinrhode2 devinrhode2 changed the title Using prettier, but seeing import {Foo} (object-curly-spacing) Using prettier, but no spaces inside braces {Foo} Oct 6, 2021
@devinrhode2
Copy link
Contributor Author

@fisker I noticed you have some PRs for eslint-plugin-prettier, which fix the arrow-body-style issues

Any thoughts here? My interest is in improving performance, but main issue here is the inconsistent formatting

@devinrhode2 devinrhode2 changed the title Using prettier, but no spaces inside braces {Foo} prettier: true, but no spaces inside braces {Foo} Oct 6, 2021
@devinrhode2 devinrhode2 changed the title prettier: true, but no spaces inside braces {Foo} prettier: true, but no spaces inside braces: {Foo} Oct 6, 2021
@devinrhode2
Copy link
Contributor Author

If prettier is just a second final step, --fix simply needs to pass final source to prettier before writing to disk.

Likewise, xo --check needs to call prettier --check

@spence-s
Copy link
Contributor

spence-s commented Oct 6, 2021

@devinrhode2 xo is only a wrapper around eslint - prettier is only ran via eslint-plugin-prettier from xo. You can configure prettier by adding a prettier field to your package.json or a .prettierrc file, you probably get this. Yes, I don't think xo follows the same defaults as prettier itself, but xo's default prettier config does NOT conflict with the xo eslint rules. - it is kind of confusing.

Best idea is to let xo manage both formatting and linting for you completely. Or turn prettier off in xo and configure prettier to manage formatting (as long as you have not set up conflicting rules)

For VSCode - you should turn the prettier plugin off for typescript and javascript if you are using the xo plugin because they will conflict. The xo plugin supports formatting on save and can be set up correctly as follows:

  "typescript.format.enable": false, // turn off vscode defaults
  "javascript.validate.enable": false,
  "editor.formatOnSave": true, // turn on formatting on save
  "[javascript]": {
	// only ever set editor.defaultFormatter under a specific language
    "editor.defaultFormatter": "samverschueren.linter-xo" 
  },
  "[typescript]": {
    "editor.defaultFormatter": "samverschueren.linter-xo"
  },

I personally set up all my VSCode linting and formatting on a per language basis as shown above.

Regarding performance in VSCode - I have some works in progress on the plugin and some updates and improvements will be coming for that soon, I just haven't quite had the time to finalize my WIP.

@devinrhode2
Copy link
Contributor Author

devinrhode2 commented Oct 6, 2021

Inside of eslint-plugin-prettier, I added log statements around the prettier.format( call:

              console.log('input', source);
              console.log('opts', prettierOptions);
              console.log('prettier version:', prettier.version);
              prettierSource = prettier.format(source, prettierOptions);
              console.log('output', prettierSource);

I'm actually noticing that eslint-plugin-prettier is getting called twice for one yarn xo --fix /path/to/file.ts

If it's given import { isDev} from './isDev' then it'll format it to import {isDev} from './isDev'

Happening with:
eslint-plugin-prettier v3.4.1, xo v0.43.0 (with some minor patching), prettier v2.3.2
eslint-plugin-prettier v4.0.0, xo v0.45.0 (with some minor patching), prettier v2.3.2
eslint-plugin-prettier v4.0.0, xo v0.45.0 (with some minor patching), prettier v2.4.1

(Inside of eslint-plugin-prettier, I'm doing console.log('prettier version:', prettier.version) - so there's no question as to what prettier version is running)

(yarn v1.22.15, node 16.10.0)

(I'm hoping calling eslint-plugin-prettier twice may be intentional, due to some bugs around arrow functions, or eslint-disable~line/next-line comments)

@devinrhode2
Copy link
Contributor Author

Also, performance inside of vscode has been great, no issues there. It's command line that is slow for me. Maybe, we don't need a pre-commit hook, but we'd still need to run xo --check inside our CI process to check for errors.

I'm am thinking that inside of xo we'd somehow directly use ./node_modules/.bin/prettier instead of require('prettier').format. I believe this would solve the problem.

Alternatively, eslint-plugin-prettier could directly use .bin/prettier. (Going to explore their issues/discuss over there some too: prettier/eslint-plugin-prettier#436)

@devinrhode2
Copy link
Contributor Author

devinrhode2 commented Oct 6, 2021

Wow, dreams come true, prettier actually has an option bracketSpacing. XO seems to be changing the default value for that. So it's just a matter of getting the right config value passed to prettier!

FTR prettier was being provided these options:

{
  // These seem to be reflection of my xo config (semi: false, space: 2)
  useTabs: false,
  tabWidth: 2,
  
  // These appear to be my options:
  semi: false,
  singleQuote: true,
  printWidth: 80,
  jsxSingleQuote: true,
  arrowParens: 'avoid',
  trailingComma: 'es5',
  quoteProps: 'consistent',
  
  // THIS is causing my issue:
  bracketSpacing: false,
  bracketSameLine: false,
}

@devinrhode2
Copy link
Contributor Author

devinrhode2 commented Oct 6, 2021

In the readme, I read this to mean that if I have a prettier config file, that is the prettier config to be used:

The Prettier options will be read from the Prettier config and if not set will be determined as follow:

Either it should say is something like:

The Prettier options will be read from the Prettier config then combined with these XO defaults:
...
If you don't like the XO defaults, you should set them to your own preferred value inside your prettier config

Alternatively - we could simply NOT set these extra prettier defaults.

@devinrhode2
Copy link
Contributor Author

I think most people using prettier will have their own prettier config. The useTabs, tabWidth both make sense for XO to be setting based on the semi, space XO config options. Other options, appear to be introduced with the original prettier PR here: #271

devinrhode2 added a commit to devinrhode2/xo that referenced this issue Oct 6, 2021
This definitely sent me down a rabbit hole: xojs#614
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

No branches or pull requests

2 participants