Bug 36400

Summary: Centralize {js,ts,vue} formatting config in .prettierrc.js
Product: Koha Reporter: Paul Derscheid <paul.derscheid>
Component: Architecture, internals, and plumbingAssignee: Paul Derscheid <paul.derscheid>
Status: RESOLVED FIXED QA Contact: Jonathan Druart <jonathan.druart>
Severity: enhancement    
Priority: P5 - low CC: blawlor, david, fridolin.somers, jonathan.druart, martin.renvoize
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00
Circulation function:
Attachments: Attachment to Bug 36400 - Centralize {js,ts,vue} formatting config in .prettierrc.js
Bug 36400: Centralize {js,ts,vue} formatting config in .prettierrc.js
Bug 36400: Centralize {js,ts,vue} formatting config in .prettierrc.js
Bug 36400 (follow-up): remove option editorconfig from .prettierrc.js
Bug 36400 (follow-up): remove option editorconfig from .prettierrc.js
Bug 36400: Centralize {js,ts,vue} formatting config in .prettierrc.js
Bug 36400 (follow-up): remove option editorconfig from .prettierrc.js

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 (ashimema) 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.