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

Use Prettier #1302

Merged
merged 7 commits into from
Jan 7, 2024
Merged

Use Prettier #1302

merged 7 commits into from
Jan 7, 2024

Conversation

ibc
Copy link
Member

@ibc ibc commented Jan 6, 2024

Details

  • Format .ts and .mjs files using Prettier (proper paths filtered).
  • New script added: npm run format:node (plus npm run format which now formats Node and Worker).

Why?

  • Because I'm tired of having to revisit tons of files and reformat them.
  • Because what I like today may not match what I like tomorrow.
  • Because it's not me alone in the project.

Bonus Tracks

  • Improve (even fix) .eslintrc.js file. For example, it was not linting npm-scripts.mjs properly.
  • Run Jest with --detectOpenHandles (no issues so far).

### Why?

- Because I'm tired of having to revisit tons of files and reformat them.
- Because what I like today may not match what I like tomorrow.
- Because it's not me alone in the project.
- New script added: `npm run format:node`

### Bonus Tracks

- Improve (even fix) `.eslintrc.js` file.
@ibc ibc requested review from jmillan and nazar-pc January 6, 2024 19:45
NodeJS: 'readonly',
},
extends: ['eslint:recommended', 'plugin:prettier/recommended'],
rules: {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not worked with these for some time, just wondering if it is necessary to have so many manual no- things in there, maybe there is a preset for this? Also not clear what has changed in the config in the way things are committed, would have been great to see one commit where file is renamed, and separate when it is changed. "change everything" approach is very hard to review.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the no-xxx options here were carefully selected in the past to satisfy our wishes and fears. Honestly I don't want to use a "preset" and adhere to it. Anyway, I'll explain in a separate comment the rationale behind changes done here.

/node/src/fbs
/rust
/target
/worker/deps
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not ignore whole /worker ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since .prettierignore uses .ignignore syntax, I'd say ignore everything and un-ignore only things that we do want to check instead, that is often a better long-term solution.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot ignore worker/ because we want to include worker/scripts/clang-format.mjs. More about this:

  • Unfortunately there is no way to tell prettier which explicit folders/paths it must observe/write via .prettierrs conf file.
  • When calling prettier command we pass the explicit list of folders/paths via argument (see formatNode() in npm-scripts.mjs):
    function formatNode() {
      logInfo('formatNode()');
    
      const paths = [
      	'npm-scripts.mjs',
      	'.eslintrc.js',
      	'node/src',
      	'worker/scripts/clang-format.mjs',
      ];
    
      executeCmd(`prettier ${paths.join(' ')} --write`);
    }
  • The problem is that we also want to make Prettier work on our text editors to automatically format code. And the text editor just runs prettier --write on any folder/file included in whatever root folder in which .prettierrs is.
  • So if I open any TypeScript file in Rust frontend-examples/ folder and edit it in my text editor, Prettier will format it, and we don't want it (at least not now since it would require that we also run ESlint on those files and that's another story).
  • So the only way to limit the forlders/paths in which Prettier works when using the text editor is by blacklisting some of them via .prettierrcignore. That's sad but that's what we have. Here a feature request about it that was rejected due to "inactivity" (as tons of other similar ones): Proposal: Filter using ".gitignore" first, then ".prettierignore" second prettier/prettier#8506
  • In the other side, Prettier is supposed to also ignore everything in .gitignore according to the docs (https://prettier.io/docs/en/ignore.html) but it's not true. Probably it only works if there is no .prettierrcignore file.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I'm suggesting then is the following in .prettierignore (if it works the way I expect it to):

*
!/npm-scripts.mjs
!/.eslintrc.js
!/node/src
!/worker/scripts/clang-format.mjs

This will ignore everything and then strategically include just a few things we actually care about.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that syntax in .prettierrcignore and it didn't work for me. Will try again.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. This .prettierrcignore file:

*
!/npm-scripts.mjs
!/.eslintrc.js
!/node/src
!/worker/scripts/clang-format.mjs
  • Doesn't ignore /npm-scripts.mjs (good).
  • Ignores any file in /node/src (bad).
  • Ignores worker/scripts/clang-format.mjs (bad).

Tested right now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very strange, I hope there is a way to make it work, that would be the cleanest solution.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I have to do to make Prettier reformat node/src/* TS files and ignore node/lib/* JS compiled files:

/*
!/npm-scripts.mjs
!/.eslintrc.js
!/node
/node/lib

Terrible, and still I don't know how to make it work with /worker/scripts/clang-format.mjs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I give up. I've tested everything. It's buggy.

…omaticlaly added to PATH when running npm commands
@ibc
Copy link
Member Author

ibc commented Jan 7, 2024

So about changes in .eslintrc.js:

  • It's been moved to root folder since it must also lint worker/scripts/clang-format.mjs.
  • It has 3 sectioons:
    1. First section is global to all JavaScript or TypeScript files.
    2. Second section is only about TypeScript files, so it adds typescript-eslint rules.
    3. Third section is only about TypeScripts files in test files, so it adds Jest stuff.
  • I added plugin:prettier/recommended in last position of the extends array in eslintrc.js as per documentation.
  • I added 'prettier/prettier': 2 in rules to make ESlint fail if some file doesn't adhere to Prettier syntax.
  • I added eslint-config-prettier package and call it in npm run lint:node task to ensure that we are not declaring any ESLint rule that is later gonna be overridden by plugin:prettier/recommended rules:
    // Ensure there are no rules that are unnecessary or conflict with Prettier
    // rules.
    executeCmd('eslint-config-prettier .eslintrc.js');

@ibc ibc merged commit bca9b0d into v3 Jan 7, 2024
20 checks passed
@ibc ibc deleted the use-prettier branch January 7, 2024 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants