feat: implement lint-staged cli option

[frbuceta]

fixes #4062

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• API Documentation in code was updated
• Documentation in /docs/site was updated
• Affected artifact templates in packages/cli were updated
• Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

[frbuceta]

I just uploaded this but please don't accept it yet.

I have a couple of doubts...

1. Is it ok what I did in utils?
2. How can I format the file? Currently not being formatted
3. How can I disable this option if eslint and prettier are disabled?

[bajtos]

Hi @frbuceta, thank you for the pull request.

Did you know that eslint has an option to cache the results of previous checks and thus speed up linting by checking only files modified from the last run? We are using the caching mode in loopback-next monorepo as can be seen here:

https://github.com/strongloop/loopback-next/blob/0ea29d872544d8b0e3908c6bcfa0efe28ac3b93e/package.json#L48

Maybe instead of introducing lint-staged, a better first step is to enable caching mode in the generated projects?

Personally, I would be reluctant to rely on git metadata to decide which files to lint. I am concerned that we can miss some files that should have been linted, especially when switching between feature branches and rebasing on top of newer master branch. In my experience, commit hooks are not very fast, which I find annoying and thus prefer to run checks when I decide on my own. Of course, that's just me, other people may have different preferences.

I also want to have a CI step to check the linting for code that I didn't wrote myself. We have a pre-commit hook to reject commit messages not following our conventions, and we are still getting pull requests with wrong commit messages. I don't know how that happens, but we must be prepared to cope with it. So even if we enable staged-lint, we still need to run the linting step as part of the CI process. In which case I find it a bit redundant to run the linting step on every commit too. (But again, that's my subjective opinion.)

Now your pull request is adding the new behavior behind a feature flag (an option the user can choose or choose not), therefore I am fine to add such feature even if I would not want to use it myself. However, I am not able to provide much feedback on the implementation details.

Is it ok what I did in utils?

It looks a bit hacky to me, but it's not the end of the world. I find it surprising that Yeoman is not removing the .ejs template automatically. But if that's how things are, then your fix looks reasonable.

How can I format the file? Currently not being formatted

Which file are you referring to?

How can I disable this option if eslint and prettier are disabled?

This is a Yeoman-related question (see https://yeoman.io). I am not very familiar with all best practices for writing Yeoman generators and the way how we are applying them to our generator hierarchy, but after a quick search I found the following place where options are handled - I think it can be a good place where to add your new code too.

https://github.com/strongloop/loopback-next/blob/c82d4d9aa11f96b2b7cb9d6c53c5fc86d935c906/packages/cli/lib/project-generator.js#L110-L136

@raymondfeng AFAIK, you are the author of ProjectGenerator.prototype.setOptions method, can you please help?

[bajtos]

Hi @frbuceta, is this pull request still relevant after we fixed eslint caching?

[dougal83]

Hi @frbuceta, is this pull request still relevant after we fixed eslint caching?

@bajtos It may still be relevant if prettier is a concern? A cached option was in the works but it was not released.

[bajtos]

@bajtos It may still be relevant if prettier is a concern? A cached option was in the works but it was not released.

Yeah, I really wish Prettier supported caching.

Few other alternatives we may want to consider:

1. Step 1: run Prettier formatting checks are part of eslint, there is (used to be?) a plugin for that
2. Step 2: run eslint checks as part of tsc build, including Prettier checks via the eslint plugin. Again, there is a plugin to run eslint as a TypeScript compiler plugin.

We need to carefully consider changes in the developer experience that would the different setup bring. E.g. do we want prettier-related formatting problems to be highlighted by VS Code when Prettier is integrated into eslint? Probably not.

[bajtos]

Another option is to use a faster alternative to Prettier, e.g. https://github.com/dprint/dprint

[frbuceta]

This has nothing to do with monitoring files in the project. It is just a package to pass the lint to the files in git. @bajtos @dougal83

I'm going to close this PR and recreate it later

[dougal83]

@frbuceta Thank you for the clarification. I apologise if my comments are off topic for the PR.

@bajtos I will have a look at the options out of interest. If it bears fruit I'll propose a PR on prettier front.