search

ui5-community / generator-ui5-project

Generator for UI5-based web-apps which use the official UI5 tooling and support multiple deployment targets such as the SAP Business Technology Platform
Apache License 2.0
33 stars 25 forks source link

Proposal to add a more sophisticated eslint ruleset #30

Closed lboehm closed 2 years ago

lboehm commented 2 years ago

Hi all,

I have two improvements regarding the eslint ruleset, currently generated by this generator:

  1. I would propose to change the rule config from integers to string. Currently it says:

    {
"block-scoped-var": 1
}

    Based on the eslint documentation, the 1 can be replaced with warn, which is easier to understand.

  2. And more importantly, I would propose to add some more rules. There are quite important ones missing, from my point of view. For example the eqeqeq rule (link). And I think as IE11 isn't supported any longer we can add eslint rules that enforce code that can't be executed by IE11 without transpilation. For example the no-var rule (link).

I've provided a PR with a changed ruleset. As this is a very important matter, we should propably discuss the changed rules. Maybe the proposed ruleset is too strict. But it can serve as a basis for a discussion.

I think we have the chance to help developers create more readable, more maintainable source code. So I would really love to have a more sophisticated ruleset. What do you guys think?

Best regards Lukas

nicoschoenteich commented 2 years ago

Hi @lboehm, Thanks for the PR. I am all for the suggested rule changes. Let's hear what the other think about it 🤗

lboehm commented 2 years ago

@IObert @vobu @petermuessig Do you guys have any thoughts about a stricter ESLint ruleset in general and the PR in particular?

vobu commented 2 years ago

i'm biased on this: personally, i like to SM me by tightening code-time rulesets (and git commit message, btw) 🤕 😆 . but i also learned that not all developers are equally pain-resistent. based on this i'd suggest:

my €.02, best, v.

lboehm commented 2 years ago

Hi @vobu

sorry for the late response, I had a busy time. Thanks a lot for your answer, this really helps.

Here's my response to your comment:

  • ruleset changes:

    • max-lines: understandable, but reality bites. there are more likely valid use cases for larger controller files than not (think operating the OData v4 model programmatically, ugh). change to warn?

Done.

  • no-var: i'd drop this one as using var instead of let is a style thingie that's ok from my POV

I think this is more than just a style thingie, as variables can't be shadowed when using let or const. So I think that's a quite useful rule, but if you still think it should be removed let me know.

  • no-plusplus: i use this all the time 😨 and also don't see a reason to enforce this with error. probably warn?

Okay, I've gone too far with this one 😃 If you like, we can remove this one completely.

  • object-shorthand: i'm all for it, but i know that @CodeWarrior is still hesitant introducing ES6 only codesamples/recommendations until the UI5 codebase goes ES6 only

I think customers can switch to ES6, even if SAP is still working on moving SAPUI5 to ES6. So from my perspective this rule would make sense. Nevertheless I've removed it.

  • dot-notation: this i'd recommend to take out as iterating over an array and dynamically calling functions from entries in that array is perfectly fine IMO. think intentionally producing async side effects in message-based architectures: arr.forEach(el => el["asyncFnName"]())

Removed it.

  • semi should definitely be dropped. it's become a question of team/project rule rather than a necessity. JS is fine with not using ";" anymore, and ASIs help w/ edge cases.

Removed it.

  • you're probably going to hate me for this, but what about sorting the rules by name? it'll be easier to follow-up long-term...

Haha no that's alright. I've done it 🙂

I've updated the PR #31, as described above. Please let me know if I should really remove the no-var rule. I didn't touch this one, because of the shadowing mentioned above.

Best regards Lukas

petermuessig commented 2 years ago

I commented the PR with some thoughts and I feel the influence of @vobu => semi! Basically, with the UI5 tooling we can allow to write the JS applications in modern JS and due to the skipped support for IE11 we can also use most of the modern JS language features and APIs. So in general, for this purpose it is good to update also the lint rules. One thing which may be problematic is the usage of the template for older releases of SAPUI5 for which still IE11 support is listed in the PAM. Most probably we do not need to support this here and they may use the older version of easy ui5.

lboehm commented 2 years ago

@petermuessig Thanks a lot for reviewing 🙂 I will check the Object spread syntax and we'll wait for @matz3 regarding ecma script version. Then I will update the PR.

petermuessig commented 2 years ago

No problem. Unfortunately, I missed to join the discussion here - sorry, I am always late to the parties. But I will try to chat with @matz3 tomorrow also tackling the object spread topic. Thanks for your work here! 👍

lboehm commented 2 years ago

Hi @petermuessig, did you get some information regarding the object spread topic? 🙂

petermuessig commented 2 years ago

Yes, see my comment in the PR. @matz3 also replied there. We are good to go with ES2020 which is supported by the UI5 tooling >=2.9.x but ES2021 require UI5 Tooling >=3.0.0-alpha.