[Bug 38524] New: Add support for Vue.js and TypeScript to ESLint config to be able to actually enforce coding guideline JS8
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 Bug ID: 38524 Summary: Add support for Vue.js and TypeScript to ESLint config to be able to actually enforce coding guideline JS8 Change sponsored?: --- Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: victor@tuxayo.net QA Contact: testopia@bugs.koha-community.org Currently ESLint doesn't try to check .vue and .ts files. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |victor@tuxayo.net |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |38167 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38167 [Bug 38167] ESLint: migrate config to flat format + cleanup some node dependencies -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 --- Comment #1 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Created attachment 174963 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=174963&action=edit Bug 38524: Enable ESLint for .ts and .vue files Test plan: 1. Apply only bug 38167 and it's dependency (assuming they are not in main) 2. Restart KTD or run sudo yarn install --modules-folder /kohadevbox/node_modules 3. Run the following and see that it doesn't lint eslint t/cypress/integration/AdditionalFields_spec.ts eslint koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/UsageStatisticsReportBuilder.vue 4. Apply the patches of this ticket 5. restart KTD (it tests that the provisionning works well to get all the JS libs) 6. Run the following and see that it lints eslint t/cypress/integration/AdditionalFields_spec.ts eslint ./t/cypress/integration/ERM/DataProviders_spec.ts eslint ./koha-tmpl/intranet-tmpl/prog/js/vue/modules/preservation.ts eslint koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/UsageStatisticsReportBuilder.vue eslint ./koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/AgreementsFormAdd.vue Various files tested here give an idea of the output, feel free to try others. And report stuff that should obviously be silenced due to making too much noise and having low value or being contrary to our current practices. Non obvious stuff to fix might be put in a follow-up ticket. --------- This was removed from config: indent: ["error", 4], Because this check was finding it redundant: npx eslint-config-prettier koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/Main.vue check which is documented here: https://github.com/prettier/eslint-config-prettier?tab=readme-ov-file#cli-he... To doubled check, I sabotaged some indentation in a .js/.ts section on these files koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/Main.vue koha-tmpl/intranet-tmpl/prog/js/ajax.js And without `indent: ["error", 4]` the prettier rule was still reporting the issue. And with `indent: ["error", 4]` the issue was reported twice. This looks enough to confirm it's redundant to keep it. ---- For the same reason as above, the following was removed: semi: ["error", "always"], tested by removing a semicolon in koha-tmpl/intranet-tmpl/prog/js/ajax.js t/cypress/integration/Auth/csrf.ts ---- This was removed: "linebreak-style": ["error", "unix"], Because it's the default of prettier https://prettier.io/docs/en/options.html#end-of-line -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 --- Comment #2 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Created attachment 174964 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=174964&action=edit Bug 38524: [DO NOT PUSH] yarn.lock changes -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 --- Comment #3 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Things that look interesting by I'm not sure if lint rules should be changed or not: eslint koha-tmpl/intranet-tmpl/prog/js/vue/components/AdditionalFieldsDisplay.vue 33:9 warning Prop "resource_type" is not in camelCase vue/prop-name-casing 34:9 warning Prop "additional_field_values" is not in camelCase vue/prop-name-casing -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 --- Comment #4 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Bonus of this: includes also prettier (guideline JS14) As for "to be able to actually enforce coding guideline JS8". There is a fair amount of tuning and deliberation before being to be able to use eslint for .vue and .ts like we are supposed to do for .js . So even with these patches we might want to amend JS8 to clarify that while Typescript and Vue files are covered by it, there are too much checks that are not relevant for us or pending work on clarifying our guidelines. So we can't by default ask everything to be followed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 --- Comment #5 from Victor Grousset/tuxayo <victor@tuxayo.net> ---
As for "to be able to actually enforce coding guideline JS8". There is a fair amount of tuning and deliberation before being to be able to use eslint for .vue and .ts like we are supposed to do for .js .
Reformulation: it raises a lot of issues in our ts and vue files. So the config doesn't match out recent practices and both need to be reconciled because it's not clear which of the two is right for us. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply CC| |david@davidnind.com --- Comment #6 from David Nind <david@davidnind.com> --- The patch no longer applies - sha1/fake ancestor error: Bug 38524 - Add support for Vue.js and TypeScript to ESLint config to be able to actually enforce coding guideline JS8 174963 - Bug 38524: Enable ESLint for .ts and .vue files 174964 - Bug 38524: [DO NOT PUSH] yarn.lock changes Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 38524: Enable ESLint for .ts and .vue files Applying: Bug 38524: [DO NOT PUSH] yarn.lock changes error: sha1 information is lacking or useless (yarn.lock). error: could not build fake ancestor Patch failed at 0001 Bug 38524: [DO NOT PUSH] yarn.lock changes Notes for applying dependencies =============================== When applying 38617 and dependencies, do them iteratively and comment out the [DO NOT PUSH] patches for the yarn lock changes. Otherwise, the patches fail to apply when you get to 38770. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 --- Comment #7 from Victor Grousset/tuxayo <victor@tuxayo.net> --- It is simply not possible to have this applying and Bug 38770 alone at the same time. Because a dependency of this (Bug 38149 but I'm not sure) conflicts with 38770 on yarn.lock. So solving the conflicts for the case of 38770 alone and the case for having 38770 in the dependency tree is not possible at the same it. Whatever, let's try to not have any yarn.lock patch. yarn install is ran in the test plans. And I don't think that even help using the sandboxes. It's just to have a clean working directory I guess? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #174964|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com QA Contact|testopia@bugs.koha-communit |martin.renvoize@ptfs-europe |y.org |.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #174963|0 |1 is obsolete| | --- Comment #8 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- Created attachment 176206 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=176206&action=edit Bug 38524: Enable ESLint for .ts and .vue files Test plan: 1. Apply only bug 38167 and it's dependency (assuming they are not in main) 2. Restart KTD or run sudo yarn install --modules-folder /kohadevbox/node_modules 3. Run the following and see that it doesn't lint eslint t/cypress/integration/AdditionalFields_spec.ts eslint koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/UsageStatisticsReportBuilder.vue 4. Apply the patches of this ticket 5. restart KTD (it tests that the provisionning works well to get all the JS libs) 6. Run the following and see that it lints eslint t/cypress/integration/AdditionalFields_spec.ts eslint ./t/cypress/integration/ERM/DataProviders_spec.ts eslint ./koha-tmpl/intranet-tmpl/prog/js/vue/modules/preservation.ts eslint koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/UsageStatisticsReportBuilder.vue eslint ./koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/AgreementsFormAdd.vue Various files tested here give an idea of the output, feel free to try others. And report stuff that should obviously be silenced due to making too much noise and having low value or being contrary to our current practices. Non obvious stuff to fix might be put in a follow-up ticket. --------- This was removed from config: indent: ["error", 4], Because this check was finding it redundant: npx eslint-config-prettier koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/Main.vue check which is documented here: https://github.com/prettier/eslint-config-prettier?tab=readme-ov-file#cli-he... To doubled check, I sabotaged some indentation in a .js/.ts section on these files koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/Main.vue koha-tmpl/intranet-tmpl/prog/js/ajax.js And without `indent: ["error", 4]` the prettier rule was still reporting the issue. And with `indent: ["error", 4]` the issue was reported twice. This looks enough to confirm it's redundant to keep it. ---- For the same reason as above, the following was removed: semi: ["error", "always"], tested by removing a semicolon in koha-tmpl/intranet-tmpl/prog/js/ajax.js t/cypress/integration/Auth/csrf.ts ---- This was removed: "linebreak-style": ["error", "unix"], Because it's the default of prettier https://prettier.io/docs/en/options.html#end-of-line Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|martin.renvoize@ptfs-europe |testopia@bugs.koha-communit |.com |y.org Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 --- Comment #9 from David Nind <david@davidnind.com> --- I had a go at testing (so that Martin's signoff could count as QA). However, after step 5 in the test plan (shutting down KTD and starting it up again) I get this error when trying to eslint any of the files: eslint ./koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/AgreementsFormAdd.vue Oops! Something went wrong! :( ESLint: 9.17.0 TypeError: Key "languageOptions": Key "globals": Global "AudioWorkletGlobalScope " has leading or trailing whitespace. at new Config (/kohadevbox/node_modules/eslint/lib/config/config.js:195:19) at [finalizeConfig] (/kohadevbox/node_modules/eslint/lib/config/flat-config-array.js:216:16) at FlatConfigArray.getConfigWithStatus (/kohadevbox/node_modules/@eslint/config-array/dist/cjs/index.cjs:1178:55) at FlatConfigArray.getConfig (/kohadevbox/node_modules/@eslint/config-array/dist/cjs/index.cjs:1196:15) at /kohadevbox/node_modules/eslint/lib/eslint/eslint.js:764:40 at async Promise.all (index 0) at async ESLint.lintFiles (/kohadevbox/node_modules/eslint/lib/eslint/eslint.js:759:25) at async Object.execute (/kohadevbox/node_modules/eslint/lib/cli.js:498:23) at async main (/kohadevbox/node_modules/eslint/bin/eslint.js:158:22) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 Bug 38524 depends on bug 38167, which changed state. Bug 38167 Summary: ESLint: migrate config to flat format + cleanup some node dependencies https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38167 What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 Bug 38524 depends on bug 38167, which changed state. Bug 38167 Summary: ESLint: migrate config to flat format + cleanup some node dependencies https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38167 What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |BLOCKED Resolution|FIXED |--- -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@gmail.com Status|Signed Off |In Discussion --- Comment #10 from Jonathan Druart <jonathan.druart@gmail.com> --- I don't understand why we are not simply using prettier, why add support for ESLint? On bug 38664 I added support to .tt, and suggested a tidy.pl script that will take care of tidying everything. With the config in .prettierrc.js for .vue, .js, .tt. What we do already. Now we will have to add the same config in another file? Please detail, it's not clear to me why we would need that at all. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #176206|0 |1 is obsolete| | --- Comment #11 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 176642 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=176642&action=edit Bug 38524: Enable ESLint for .ts and .vue files Test plan: 1. Apply only bug 38167 and it's dependency (assuming they are not in main) 2. Restart KTD or run sudo yarn install --modules-folder /kohadevbox/node_modules 3. Run the following and see that it doesn't lint eslint t/cypress/integration/AdditionalFields_spec.ts eslint koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/UsageStatisticsReportBuilder.vue 4. Apply the patches of this ticket 5. restart KTD (it tests that the provisionning works well to get all the JS libs) 6. Run the following and see that it lints eslint t/cypress/integration/AdditionalFields_spec.ts eslint ./t/cypress/integration/ERM/DataProviders_spec.ts eslint ./koha-tmpl/intranet-tmpl/prog/js/vue/modules/preservation.ts eslint koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/UsageStatisticsReportBuilder.vue eslint ./koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/AgreementsFormAdd.vue Various files tested here give an idea of the output, feel free to try others. And report stuff that should obviously be silenced due to making too much noise and having low value or being contrary to our current practices. Non obvious stuff to fix might be put in a follow-up ticket. --------- This was removed from config: indent: ["error", 4], Because this check was finding it redundant: npx eslint-config-prettier koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/Main.vue check which is documented here: https://github.com/prettier/eslint-config-prettier?tab=readme-ov-file#cli-he... To doubled check, I sabotaged some indentation in a .js/.ts section on these files koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/Main.vue koha-tmpl/intranet-tmpl/prog/js/ajax.js And without `indent: ["error", 4]` the prettier rule was still reporting the issue. And with `indent: ["error", 4]` the issue was reported twice. This looks enough to confirm it's redundant to keep it. ---- For the same reason as above, the following was removed: semi: ["error", "always"], tested by removing a semicolon in koha-tmpl/intranet-tmpl/prog/js/ajax.js t/cypress/integration/Auth/csrf.ts ---- This was removed: "linebreak-style": ["error", "unix"], Because it's the default of prettier https://prettier.io/docs/en/options.html#end-of-line Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |25.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 --- Comment #12 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 25.05! Well done everyone, thank you! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |This updates the release notes| |configuration for ESLint | |(used for finding | |JavaScript coding errors) | |so that ESLint can be used | |to check Vue (.vue) and | |TypesScript (.ts) files. | |This will help with | |enforcing coding guideline | |JS8: Follow guidelines set | |by ESLint | |(https://wiki.koha-communit | |y.org/wiki/Coding_Guideline | |s#JS8:_Follow_guidelines_se | |t_by_ESLint). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |baptiste.wojtkowski@biblibr | |e.com --- Comment #13 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Linter improvements, not backported -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |Needs documenting -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38524 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs documenting |RESOLVED Resolution|--- |FIXED --- Comment #14 from David Nind <david@davidnind.com> --- No changes required to the manual. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org