search

pkp / pkp-lib

The library used by PKP's applications OJS, OMP and OPS, open source software for scholarly publishing.
https://pkp.sfu.ca
GNU General Public License v3.0
297 stars 441 forks source link

Add additional vue eslint rules #10153

Open jardakotesovec opened 3 days ago

jardakotesovec commented 3 days ago

Describe the issue Based on the last code review from @ewhanson, I am suggesting to add two new eslint rules, which help to catch following inconsistency.

1) Naming components in the vue files. Historically we do have lots of kebab-case naming of components, like <pkp-button>. I would like to follow vue recommendation, which is to use PascalCase in *.vue files. Therefore becomes <PkpButton>.

Apart just from following recommendation I think this makes nice differentiation between native html tags and vue components. Only situation where vue recommends to use kebab-case is when the templates are defined within DOM - which in our case are the smarty templates. So we need to stick with it there for technical reasons. But given that we are moving quite fast from these templates I don't see this as issue.

2) Second rule checks that all components that are used in templates were imported. We do have lots of basic components registered globally, so it was not necessary to always import&register them, which resulted in inconsistent situation when sometime its imported and sometime not.

This second role ensures its always imported, which helps avoid mistakes when we forget import component that is not globally registered. And also will be likely useful in foreseeable future for chromatic functionality, which based on the imports it evaluates components dependencies and run snapshot only on component which has changed or their dependency has changed.

Also to mention that the global registered components are meant to be exposed for plugins. This might also evolve in future how do we expose these components to the plugin. Therefore not relying on them in our codebase is also beneficial.

Here is PR for ui-library: https://github.com/pkp/ui-library/pull/375

No need to code review - but in case you have suggestions or opinions before I merge it, let me know: @ewhanson @jonasraoni @asmecher @blesildaramirez @taslangraham

What application are you using? OJS, OMP or OPS version main

taslangraham commented 3 days ago

@jardakotesovec The added rules look good. Just a general question, are we using multiple-word component names as suggested in vue styleguide?

jardakotesovec commented 2 days ago

@taslangraham That would be good one - but hard to adopt it at this point as we have already way to many one word components..

jardakotesovec commented 2 days ago

@taslangraham thats why its explicitely disabled in our eslint config..

jardakotesovec commented 2 days ago

Merged

blesildaramirez commented 2 days ago

Hi @jardakotesovec , I did a review of the changes on the PR https://github.com/pkp/ui-library/pull/375. All the changes for the vue component files look good. However, it would also be good to also update our story and documentation files (.mdx) to reflect these changes.

jardakotesovec commented 2 days ago

@blesildaramirez Thank you. Did not think of that. Will check bit later how feasible is to maybe automate some of it. It would be good to update it, so its not confusing.