search

redwoodjs / redwood

The App Framework for Startups
https://redwoodjs.com
MIT License
17.13k stars 979 forks source link

[RFC]: Linting Generated Files #6223

Open pantheredeye opened 2 years ago

pantheredeye commented 2 years ago

Summary

Should generated files be linted?

(This seems to get into the realm of: prettier vs linter and "... In other words, use Prettier for formatting and linters for catching bugs!".)

A) If so, what approach to take? We currently have two proposed paths forward for this option.

  1. Lint all generated files at runtime: yarn rw lint --fix --path <file>
  2. Lint all files in CI Pipeline

B) If not - what other approaches should be considered to clean up issues such as #1551, #6097 or #4824?

Motivation

Linting generated files has come up in different ways across multiple issues. A clear path has not been settled on and we are looking for consensus.

Option A (run lint --fix on generated files):

1) The 'easy' way is to run yarn rw lint --fix on all generated files (with an option to disable). #6218 PR was filed this way to get something going. However, this slows down the cli and goes against #6027.

2) Clean up inside the framework by running yarn rw lint --fix on generators in the CI pipeline. @Philzen proposed this with "If it doesn't come back with exit code 0, the pipeline fails and we have identified a todo 🎉" More detailed entry here: https://github.com/redwoodjs/redwood/pull/6059#issuecomment-1207528290.

Personally, out of these two options, I like something along the lines of the second idea. It feels like good housekeeping to keep the framework clean vs pushing cleanup to the client.

Option B (other options):

Prettier seems to be the answer to the big whitespace issue, not lint. But, as @orta mentions, the files seem to go through prettier and there are still issues?

Maybe this could be rectified by just running the generated file through Prettier? Is that something we can even do?

Originally posted by @cannikin in https://github.com/redwoodjs/redwood/issues/6097#issue-1324753803

Given that these errors seem to be mostly prettier errors as far as I've seen, this issue could be reduced in scope to be just prettier which can be guaranteed to avoid the potential for non-autofixable errors.

( Interesting, I read the source code, and found that templated files were ran through prettier )

Originally posted by @orta in https://github.com/redwoodjs/redwood/issues/1551#issuecomment-939868546

Detailed proposal

If Option A, what is the ideal solution, potential problems, gotchas, etc?

If Option B, what are other solutions or ideas?

Selected comments from previous discussions:

I think we should (and do) lint/format everything we generate, but that can happen in the framework as opposed to the redwood app, unless you meant you have different eslint/prettier settings that you want the generators to inherit?

Originally posted by @jtoar in https://github.com/redwoodjs/redwood/issues/1551#issuecomment-845515064

I also recently contemplated about how to avoid templates producing code that violates linting rules, as this comes up over and over again. I can only assume currently there must be numerous templates that still produce code that doesn't comply since the new import order sort rule was introduced in 2.0.

Running lint --fix after every generator command may be an easy fix, but it would (1) add to the execution time and (2) sometimes there are cases which aren't auto-fixable, which i encountered during upgrading our project's code base to 2.x.

I wondered: isn't create-redwood-app part of the CI pipeline? If not, maybe we can add it, generating a plain project, then trying out all generator commands that are not XOR (such as the auth or setup ui stuff) and then running yarn rw lint. If it doesn't come back with exit code 0, the pipeline fails and we have identified a todo :tada: … The XOR stuff could then be tested step by step in the same fashion.

Originally posted by @Philzen in https://github.com/redwoodjs/redwood/issues/6097#issuecomment-1207316225

  • Generators, by design, are boilerplate and will include code that might be useful including examples, vars, comments, etc. This will result in lint failures by design, e.g. "unused vars"
  • It is expected devs will review, modify, remove, code from the generators for all file types — and we should "enforce" this behavior
  • Although we can't fix formatting errors ahead of time via templates (imagine a long file name that requires a line break whereas a shorter one would not), we can run yarn rw lint --fix anytime a generator is run (and also swallow the error output)
  • we can do a better job communicating all this as well

Originally posted by @thedavidprice in https://github.com/redwoodjs/redwood/issues/4824#issuecomment-1074106957

We already have default config for ESLint and Prettier. So what happens is that the generated code, using templates, sometimes doesn't conform to formatting, which means it immediately has a VS Code warning/error. This is annoying/confusing. What's more, it can't always be fixed within the template. E.g. sometimes a new import is needed — the generator just adds to the bottom of the existing imports but ESLint might get angry 'cause "not grouped correctly!".

The solution here is effectively to run:

  • yarn rw g scaffold <model> (or any generator) followed by
  • yarn rw lint --fix

However, it's probably not as easy as globally running lint --fix:

  • we should consider whether or not it's OK to fix any/all pre-existing issues
  • there are many cases where generator boilerplate has unused vars
  • ... ?

Originally posted by @thedavidprice in https://github.com/redwoodjs/redwood/issues/1551#issuecomment-897309543

Would be great if we could lint --fix and prettier format all our generated code! I think that's the only sustainable way forward here, rather than trying to manually fix all our templates, and then keeping them that way.

Originally posted by @Tobbe in https://github.com/redwoodjs/redwood/issues/6097#issuecomment-1202660817

Are you interested in working on this?

Tobbe commented 2 years ago

Although we can't fix formatting errors ahead of time via templates (imagine a long file name that requires a line break whereas a shorter one would not), we can run yarn rw lint --fix anytime a generator is run (and also swallow the error output)

I don't think this is linting, right? That's formatting done by prettier.

So reading all the arguments above I would propose:

One thing we can't take into account here is the user's linting rules. We could have done that if we ran the linter as part of the generator. But we can probably use the user's prettier rules.

Philzen commented 2 years ago

Just to outline what i'm thinking of:

Option B1

Add yarn rw lint (without --fix) to the right CI job(s) (guess only one or two them will do). This way the job will fail with an exit code of <> 0 and thus require to fix any existing code, as well as templates and code generators to only produce flawless code before something can be merged. As @pantheredeye said:

It feels cleaner to address linting issues before code ever hits the main branch.

This would also include removing --fix from the fixture generation script (yarn build:test-project --rebuild-fixture). That way we'd also get rid of an inconsistency, where the fixture code is automagically fixed, while the template code may still violate linting rules and thus look different, which does not feel correct.

BTW – we should also have the lint command include all {ts|tsx|js|jsx}*.template-files.

Of course, new generators and templates would need to be tracked and included in the CI test scripts as they are implemented. Would be cool if we could come up with a configuration framework that shows which generator commands are already covered and which aren't, so new ones also make the CI pipeline fail unless they get included (i.e. into the smoke test pipeline).

This option surely requires a larger PR to get rid of each and every linting rule violation in code, templates and generated code first – but once in place we can finally ensure Redwood mainline always contains, respectively creates perfectly formatted, lint-flawless code.

Here is a (non-complete) list of issues that we would probably never have to deal with again once such checks are in place, b/c the offending code would never make it through the CI checks:

Tobbe commented 2 years ago

BTW – we should also have the lint command include all {ts|tsx|js|jsx}*.template-files.

Unfortunately I don't think that's feasible. A lot of them have a bunch of logic in them that makes the syntax invalid js/ts. For example https://github.com/redwoodjs/redwood/blob/main/packages/cli/src/commands/generate/sdl/templates/sdl.ts.template

Tobbe commented 2 years ago

@Philzen This is another one for your list I think https://github.com/redwoodjs/redwood/pull/6221 And please weigh in with your opinion on that one too, if you have one

pantheredeye commented 2 years ago

Danny note:

If taking approach A.1: If lint fails during generate process, the generator will fail without giving a clear understanding of why. If this approach is taken, then we could swallow the error.