fix(ci): fix ci lint step

[Shinigami92]

Description

fixes #2986

Additional context

The CI runs build-vite and this triggers the prebuild of the vite package
This is currently done because it safes one script step for the release process

Todos:

• Lint CI run lint step and report errors
• release script perform linting
• pre-commit-hocks perform linting

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[Shinigami92]

Forcefully thrown a lint error in this CI: https://github.com/vitejs/vite/pull/2988/checks?check_run_id=2342427599#step:5:21

[Shinigami92]

Interesting note: the removed disable-lint-line only throws an error if the package wasn't build yet 😅
Cause it requires a file from dist folder

Therefore the example is a bit bad, cause CI succeeds with it 😄

@patak-js this raises the discussion in my head: should the lint be done before or after the build? should it depend on build?
... Mh 🤔 I think no, it should not depend on build and also work without building the project in IDEs

[Shinigami92]

The forcefully thrown lint warning is shown correctly in the files changes tab

[Shinigami92]

@patak-js could you provide initial feedback for commit 04e80a6

I may add a step after that to ask for proceed or not to proceed 🤔

I'm I on the right direction at all?

[patak-dev]

You know a lot more about this than me, I trust your judgment here. If you need feedback from others, maybe Anthony or other members have more experience.

[Shinigami92]

Ping @antfu 🙂

[nihalgonsalves]

Small comment, but I think the approach makes sense 👍🏼

[Shinigami92]

This PR would also fix occurrences of multiple lint warnings in a PR like this:

https://github.com/vitejs/vite/pull/1992/files#diff-7ea20ab586e000d69e1adb4aeb7e126745c763f6d15f30e09f8ca2149722e6f1R38

[patak-dev]

Is this still a draft @Shinigami92 or we could merge it?

[Shinigami92]

I had this still as a draft because I wasn't sure myself if I covered everything

But IMO we can merge it / it will not break anything, and still if... You know where to contact me 😆

[patak-dev]

@Shinigami92 we discussed this with Anthony and it is better to avoid changing the release process. Isn't linting already being checked during prebuild anyways and in the CI? In any case, I think it would be good that we are able to merge this so if you think this is important maybe we could discuss it in a separate PR.

[antfu]

Yeah, thinking we can just remove this addition to the release script and the rest LGTM.

[Shinigami92]

I added this especially here, cause I removed it from the package.json's prebuild!
Also (I don't know why), but in special cases the lint of packages/vite:lint catches some other lint as the mono-root lint. That may have to do something with building the dist folder and checking if it was build so it can be required.

It needs to be removed from the prebuild step, cause it would always be triggered in the CI and therefore trigger linting multiple times (see #2988 (comment))

And yeah, linting is not that relevant as tests, but it safes a bit more stability
If I understand it correctly, if the lint fails here (exit code 1), then only the version number in package.json would be bumped up, but only locally and it wasn't committed or tagged already to somewhere. So this is not a problem at all.

---

Should I still remove it?

[patak-dev]

Another option could be to have a ci-build step that doesnt lint

[Shinigami92]

Could you explain/propose a bit more? prebuild is just called before build by npm automatically. That's the main issue here currently.

[patak-dev]

I think we should keep linting in prebuild not because of the release process, but because it helps when modifying Vite before sending a PR. It is a good idea to me that the build step lints (in the same way that it checks typing) so people can feel more confident before sending a PR. I don't think we should force them to run the linting on the side (nobody would remember). We had this discussion with @Shinigami92, where the focus was the release process, but to me what is important is the regular build process. If you would like to remove linting from release, I think it is fine, but let's avoid removing it from prebuild.
About the ci- scripts, that is one way to achieve this, and it is already being used before this PR in ci-docs for example.

[Shinigami92]

But we have pre commit hooks for this? 🤔

vite/package.json

Lines 57 to 72 in 6dde32a

	"gitHooks": {
	"pre-commit": "lint-staged",
	"commit-msg": "node scripts/verifyCommit.js"
	},
	"lint-staged": {
	"*.js": [
	"prettier --write"
	],
	"*.ts": [
	"eslint",
	"prettier --parser=typescript --write"
	],
	"*.html": [
	"prettier --write"
	]
	}

[patak-dev]

Ok, your call then 👍🏼

[Shinigami92]

I could try (later in another PR) to use pre-commit with prettier --write and eslint for everything
Also for md and other files, so we have a consistent code base over the whole repo

I think I will come this evening to have a last look into this PR and make a decision

[Shinigami92]

I created a task for me to check prettier and eslint: #3261
IMO lets just merge this PR for now, so the main issue of multiple occurrences in PR reviews are solved