Bug 36400 - Centralize {js,ts,vue} formatting config in .prettierrc.js
Summary: Centralize {js,ts,vue} formatting config in .prettierrc.js
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Paul Derscheid
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-03-22 11:14 UTC by Paul Derscheid
Modified: 2024-07-05 10:04 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00


Attachments
Attachment to Bug 36400 - Centralize {js,ts,vue} formatting config in .prettierrc.js (6.81 KB, patch)
2024-03-22 11:41 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 36400: Centralize {js,ts,vue} formatting config in .prettierrc.js (6.86 KB, patch)
2024-03-22 15:03 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 36400: Centralize {js,ts,vue} formatting config in .prettierrc.js (6.94 KB, patch)
2024-03-25 13:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36400 (follow-up): remove option editorconfig from .prettierrc.js (916 bytes, patch)
2024-03-25 14:00 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 36400 (follow-up): remove option editorconfig from .prettierrc.js (970 bytes, patch)
2024-03-25 15:11 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 36400: Centralize {js,ts,vue} formatting config in .prettierrc.js (6.94 KB, patch)
2024-04-02 09:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36400 (follow-up): remove option editorconfig from .prettierrc.js (1.02 KB, patch)
2024-04-02 09:56 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Derscheid 2024-03-22 11:14:14 UTC
I think a prettier config file is the way to go here.

We need to ignore all formatting rules of eslint or we might get performance issues.

Please correct component if wrong.
Comment 1 Paul Derscheid 2024-03-22 11:26:33 UTC
Using the cli .editorconfig should be ignored by default, see https://github.com/prettier/prettier/issues/15401
Comment 2 Paul Derscheid 2024-03-22 11:41:41 UTC
Created attachment 163695 [details] [review]
Attachment to Bug 36400 - Centralize {js,ts,vue} formatting config in .prettierrc.js

Bug 36400: Centralize {js,ts,vue} formatting config in .prettierrc.js

    Let's centralize all formatting for js, ts and vue files in a .prettierrc.js.
    The goal here is to preserve eslint's linting but ignore all of its formatting options.

    Seems to work for vue files in vscode, vscodium and neovim w/ vscode-eslint-language-server.

    To test:
    1) Pick any vue file for example.
    2) Run format via your editor and either npx prettier -c .prettierrc.js <PATH_TO_VUE_FILE> [you can also use the -w flag to directly write].
    3) Look for unexpected changes: vue files should be already formatted w/ these inline flags (see: https://wiki.koha-community.org/wiki/Coding_Guidelines#JS14:_Prettier_JavaScript): [--trailing-comma es5, --arrow-parens avoid].
    4) Sign off if you're happy and comment or show what went wrong in a comment.
Comment 3 Brendan Lawlor 2024-03-22 15:03:11 UTC
Created attachment 163710 [details] [review]
Bug 36400: Centralize {js,ts,vue} formatting config in .prettierrc.js

Let's centralize all formatting for js, ts and vue files in a .prettierrc.js.
    The goal here is to preserve eslint's linting but ignore all of its formatting options.

    Seems to work for vue files in vscode, vscodium and neovim w/ vscode-eslint-language-server.

    To test:
    1) Pick any vue file for example.
    2) Run format via your editor and either npx prettier -c .prettierrc.js <PATH_TO_VUE_FILE> [you can also use the -w flag to directly write].
    3) Look for unexpected changes: vue files should be already formatted w/ these inline flags (see: https://wiki.koha-community.org/wiki/Coding_Guidelines#JS14:_Prettier_JavaScript): [--trailing-comma es5, --arrow-parens avoid].
    4) Sign off if you're happy and comment or show what went wrong in a comment.

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 4 Brendan Lawlor 2024-03-22 15:07:41 UTC
Tested using vscode:

Before the patch:
npx prettier ./koha-tmpl/intranet-tmpl/prog/js/vue/components/KohaTable.vue

git diff --compact-summary ./koha-tmpl/
intranet-tmpl/prog/js/vue/components/KohaTable.vue
 .../prog/js/vue/components/KohaTable.vue          | 110 +++++++++----------
 1 file changed, 55 insertions(+), 55 deletions(-)

After the patch:
npx prettier -c .prettierrc.js -w ./koha-tmpl/intranet-tmpl/prog/js/vue/components/KohaTable.vue

Checking formatting...
[warn] Ignored unknown option { editorconfig: "false" }.
[warn] Ignored unknown option { editorconfig: "false" }.
All matched files use Prettier code style!

No changes made to the vue file
Comment 5 Jonathan Druart 2024-03-25 12:35:07 UTC
Can you explain why we need eslint deps whereas we already have prettier?
Comment 6 Paul Derscheid 2024-03-25 12:36:57 UTC
Yes, using eslint and prettier together can cause performance problems because if not mitigated they both apply their formatting rules which can cause very sluggish formatting and even timeouts.
Comment 7 Jonathan Druart 2024-03-25 13:02:03 UTC
Created attachment 163799 [details] [review]
Bug 36400: Centralize {js,ts,vue} formatting config in .prettierrc.js

Let's centralize all formatting for js, ts and vue files in a .prettierrc.js.
    The goal here is to preserve eslint's linting but ignore all of its formatting options.

    Seems to work for vue files in vscode, vscodium and neovim w/ vscode-eslint-language-server.

    To test:
    1) Pick any vue file for example.
    2) Run format via your editor and either npx prettier -c .prettierrc.js <PATH_TO_VUE_FILE> [you can also use the -w flag to directly write].
    3) Look for unexpected changes: vue files should be already formatted w/ these inline flags (see: https://wiki.koha-community.org/wiki/Coding_Guidelines#JS14:_Prettier_JavaScript): [--trailing-comma es5, --arrow-parens avoid].
    4) Sign off if you're happy and comment or show what went wrong in a comment.

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 8 Paul Derscheid 2024-03-25 14:00:24 UTC
Created attachment 163818 [details] [review]
Bug 36400 (follow-up): remove option editorconfig from .prettierrc.js

- The option doesn't seem do what it's intended for:
	- The debug logs still show options from the .editorconfig (which is actually what we want in this case).
- The option causes unwanted warnings when using the cli.
- In summary: we have to keep in mind that the rules are now hierarchical:
	1. .editorconfig
	2. .prettierrc.js
Comment 9 Brendan Lawlor 2024-03-25 14:39:28 UTC
Is the test plan for the follow up the same as it was for the first patch?
Comment 10 Paul Derscheid 2024-03-25 14:41:39 UTC
Yes, it's exactly the same.
Comment 11 Brendan Lawlor 2024-03-25 15:11:34 UTC
Created attachment 163825 [details] [review]
Bug 36400 (follow-up): remove option editorconfig from .prettierrc.js

- The option doesn't seem do what it's intended for:
	- The debug logs still show options from the .editorconfig (which is actually what we want in this case).
- The option causes unwanted warnings when using the cli.
- In summary: we have to keep in mind that the rules are now hierarchical:
	1. .editorconfig
	2. .prettierrc.js

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 12 Brendan Lawlor 2024-03-25 15:13:50 UTC
Before patch:
npx prettier -w ./koha-tmpl/intranet-tmpl/prog/js/vue/components/KohaTable.vue

git diff --compact-summary ./koha-tmpl/
intranet-tmpl/prog/js/vue/components/KohaTable.vue
 .../prog/js/vue/components/KohaTable.vue          | 110 +++++++++----------
 1 file changed, 55 insertions(+), 55 deletions(-)

After patch:
npx prettier -c .prettierrc.js -w ./koha-tmpl/intranet-tmpl/prog/js/vue/components/KohaTable.vue
Checking formatting...
All matched files use Prettier code style!
Comment 13 Jonathan Druart 2024-04-02 09:56:06 UTC
Created attachment 164248 [details] [review]
Bug 36400: Centralize {js,ts,vue} formatting config in .prettierrc.js

Let's centralize all formatting for js, ts and vue files in a .prettierrc.js.
    The goal here is to preserve eslint's linting but ignore all of its formatting options.

    Seems to work for vue files in vscode, vscodium and neovim w/ vscode-eslint-language-server.

    To test:
    1) Pick any vue file for example.
    2) Run format via your editor and either npx prettier -c .prettierrc.js <PATH_TO_VUE_FILE> [you can also use the -w flag to directly write].
    3) Look for unexpected changes: vue files should be already formatted w/ these inline flags (see: https://wiki.koha-community.org/wiki/Coding_Guidelines#JS14:_Prettier_JavaScript): [--trailing-comma es5, --arrow-parens avoid].
    4) Sign off if you're happy and comment or show what went wrong in a comment.

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 14 Jonathan Druart 2024-04-02 09:56:09 UTC
Created attachment 164249 [details] [review]
Bug 36400 (follow-up): remove option editorconfig from .prettierrc.js

- The option doesn't seem do what it's intended for:
	- The debug logs still show options from the .editorconfig (which is actually what we want in this case).
- The option causes unwanted warnings when using the cli.
- In summary: we have to keep in mind that the rules are now hierarchical:
	1. .editorconfig
	2. .prettierrc.js

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Katrin Fischer 2024-04-22 07:06:12 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 16 Katrin Fischer 2024-04-22 08:00:27 UTC
It looks like we are missing something here:

t/Makefile.t
not ok 1 - All directories should be mapped: .prettierrc.js
Comment 17 Martin Renvoize 2024-04-24 09:14:33 UTC
I have the test fix locally and will push shortly.. removing additional_work_needed.
Comment 18 Fridolin Somers 2024-05-23 14:39:41 UTC
Not backported to 23.11.x
Comment 19 David Nind 2024-07-05 10:04:53 UTC
No changes required to the manual.

If changes are required to the coding guidelines (https://wiki.koha-community.org/wiki/Coding_Guidelines), please update them.