[BREAKING ENHANCEMENT] Refactor generate script to only lint generated files

[Jan-PieterBaert]

This pull request changes the package.json file.
With this pull request when generating files, the lint:fix script will be executed on only the generated files instead of the entire project.

[madnificent]

This has been discussed on the Discord April of 2022. Thanks for implementing @Jan-PieterBaert

Before this PR, Prettier ran on all code when running a generator. This is surprising for the code-bases where prettier isn't fully adopted yet and it hinders the adoption for prettier for people with slower machines.

The motivation as to why it's okay to run Prettier on the whole code-base is because the code-base should be fully compliant either case. For that situation, this change is not a breaking change and it is only a performance improvement.

Hindrance of prettier adoption on limited hardware

In the chat conversation it became clear that at least one user (and probably more) disable Prettier because it makes developing EmberJS on a large codebase too slow. One of the reasons this may be is because a generator will run prettier on all the code, thus making the generation process substantially slower than what it should be. This PR will therefore make running generators on large projects faster.

Unexpected changed files

When happily hacking an EmberJS codebase that isn't fully Prettier compatible yet, a user (like me) may notice suddenly a bunch of files have changed. The reader of this PR will know it comes from running ember generate somewhere but it's not obvious for the casual developer. The changed files listing of the generate command doesn't include the changes Prettier has made when running the generator. The casual developer notices the changed files when preparing their commit. At that point the user notices all files have been ran through Prettier but it is not clear where this came from (at least, it was not for me) and it becomes harder to write a clear commit message with only the relevant changes.

[bertdeblock]

The lint:fix script now assumes every lint:*:fix script needs to run on the entire codebase because of the trailing ..

I'm not sure if that's an assumption we can make.
E.g. when you have a custom script like "lint:json:fix": "prettier \"./**/*.json\" --write", instead of running prettier \"./**/*.json\" --write", this will run prettier \"./**/*.json\" --write" . and will most likely fail / produce unexpected results as the trailing . supersedes \"./**/*.json\".

Also:

• This removes the --print-name flag which was just added over in [BUGFIX] - Address npm-run-all and Yarn 3 conflict & Removed warning #9988
• -- is not needed when using Yarn, using this results in the following warning:

  From Yarn 1.0 onwards, scripts don't require "--" for options to be forwarded. In a future version, any explicit "--" will be forwarded as-is to the scripts.
  

[Jan-PieterBaert]

a solution for this could be to make a :script version of the current lint:js:fix and lint:hbs:fix scripts which would take the files to run on as argument (with the . being the default used when lint:js:fix and lint:hbs:fix are executed) and then running lint:*:fix:script instead of lint:*:fix

[bertdeblock]

The :script suffix feels unnecessary to me for a script name. Also, this script seems like it should not be run from the terminal, only by Ember CLI itself (?), which makes me wonder if it belongs here?

[Jan-PieterBaert]

The :script suffix is just to make lint:fix still executable as before, I put it here since I didn't know where to put it elsewhere, where do you think it would fit better than here?

[bertdeblock]

It seems removing the . makes the script unusable via the terminal. It doesn't fix lint violations anymore.

[Jan-PieterBaert]

would solving this by making a :script version be the preferred way?

[bertdeblock]

It seems removing the . makes the script unusable via the terminal. It doesn't fix lint violations anymore.

[Jan-PieterBaert]

would solving this by making a :script version be the preferred way?

[bertdeblock]

Also, just noticed that this sends every file to every lint:*:fix script, which results in (parsing) errors.
E.g. .js files are sent to ember-template-lint and .hbs files are sent to eslint.

I'll try to check on Wednesday with the rest of the team, what would be the best way forward.

[bertdeblock]

I understand/acknowledge the motivation behind this PR, but I'm not convinced that this is the right solution. Admittedly, I'm not sure what the right solution would be here, but personally, I wouldn't want to force these additional :script scripts on every app and addon. Apps and addons that do rely on post-blueprint-generation linting would also have to add an additional :script script for each custom fix script that they have.

I'm also slightly "concerned" about the fact that now every generated file is passed along to every fix script (e.g. .js files to ember-template-lint, .hbs to eslint, ...). It could be that most of these linting tools handle this gracefully, but I'm unsure what this would give for apps and addons with additional/custom fix scripts. It just feels a bit brittle to rely on and I think it's safer that every script is responsible for its own glob.

[bertdeblock]

This was discussed during the CLI meeting. Everyone agreed that the current state is not ideal, but we also felt that the proposed solution would be to invasive/questionable for apps and addons and that your best bet at the moment would be to simply disable the functionality if it doesn't fit your workflow.

This can be done via your .ember-cli file:

{
  "lintFix": false
}

Thanks though for putting time and effort into this.

[bertdeblock]

One other "alternative" could be to have this functionality disabled by default and have users opt-in instead, but I assume we would need more community feedback on this before making such a decision.

[bertdeblock]

Also made a suggestion on Discord to improve the current situation a bit feedback-wise:
https://discord.com/channels/480462759797063690/480501885837770763/1025092185839894578

[madnificent]

@bertdeblock If I understand correctly, setting lintFix to false would mean all lint fixes are gone, which in turn means generated files will have a challenging structure (especially when blueprints expect linting to occur). It would also not help newcomers as they will not set this property and there is no obvious way of discovering when linting has ran and therefore also no obvious way of discovering this setting as a solution.

I also understand the solution provided here is not as clean as it should be and I don't see an obvious path forward.

Is there a different way to expose linting which could be used from blueprint generation?

[bertdeblock]

Setting lintFix to false would indeed disable the entire functionality, though normally, the official blueprints should already be formatted properly (unless your linting/formatting rules deviate heavily from what's in the default app/addon blueprint).

Is there a different way to expose linting which could be used from blueprint generation?

Not that I know of unfortunately, but maybe you can update the lint:fix script in your project to only lint changed files. I guess that's info you could get from Git.

[madnificent]

We ourselves are looking at a 100-something repositories with Ember code in them. We are looking at a solution with EmberJS as a whole whenever possible. Automated patches also don't tend to translate to new projects and on-boarding of new people.

The git suggestion you make is a very interesting one though.

Would an optional argument to lint:fix to only take into account files changed as per Git make sense? We could leverage that to limit the scope of the problem. This would especially help towards speed (though that's not something I feel myself). With the nice upgrades to the output you posted on Discord this could also help lower confusion.

I'd still be looking for a more sturdy solution but using git changed files would help a bit?