Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: run tscheck on babel 8 breaking test #14190

Merged

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Jan 20, 2022

Q                       A
Fixed Issues? Run tscheck on Babel 8 breaking tests
License MIT

Currently we only run eslint on the Babel 8 build. In this PR we also enable tscheck. I will mark the PR ready to review when CI is green.

@babel-bot
Copy link
Collaborator

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/50854/

@JLHwung JLHwung marked this pull request as ready for review January 20, 2022 20:50
@nicolo-ribaudo
Copy link
Member

I don't think that this is needed, or at least I don't understand why it's needed.

The BABEL_8_BREAKING (+ STRIP_BABEL_8_FLAG) only affects the compiled files and not the source files. We run linting and type-checking on the source files, which are not affected by those two flags.

However, when linting we use @babel/eslint-parser and some ESLint plugins directly from our repo: even if the source files are not affected by BABEL_8_BREAKING, it's the linter itself that changes behavior between Babel 7 and Babel 8. For this reason, running make lint in Babel 8 tests is valuable because the ESLint behaviour might change.

This reasoning doesn't affect type checking: the type checker is the same when using Babel 7 and when using Babel 8, so (given that the src files are the same) it should produce the same result.

@JLHwung
Copy link
Contributor Author

JLHwung commented Jan 21, 2022

The BABEL_8_BREAKING flag does affect sources. For example we have removed t.Noop in Babel 8 so path.isNoop is not defined in packages/babel-traverse/src/path/generated/validators.ts. If path.isNoop() is used somewhere, it will pass in Babel 7 but fail in Babel 8.

@existentialism existentialism added the PR: Internal 🏠 A type of pull request used for our changelog categories label Jan 22, 2022
@nicolo-ribaudo nicolo-ribaudo merged commit 48cf642 into babel:main Jan 22, 2022
@nicolo-ribaudo nicolo-ribaudo deleted the run-tscheck-on-babel-8-breaking-test branch January 22, 2022 15:23
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Apr 24, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Internal 🏠 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants