test(linter/plugins): use @typescript-eslint/typescript-estree package directly in conformance build#16774
Conversation
github-actions
bot
added
A-linter
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
The biggest concern is snapshot brittleness: embedding pnpm’s full virtual-store paths (with exact versions) will cause frequent, noisy churn on dependency updates. In tokens.ts, the conformance-only conditional require() should have a more actionable failure mode (and optionally a fallback) to reduce confusing runtime breakage. The TODO comment could be clarified to reflect whether the long-term goal is “back to bundle” or “bundle with good stack traces.”
Additional notes (2)
- Readability |
apps/oxlint/src-js/plugins/tokens.ts:153-163
This branch relies on a build-timeCONFORMANCEflag to decide which module to load. Two risks to watch:
- If
CONFORMANCEis ever true in an environment where@typescript-eslint/typescript-estreeisn’t installed/available (e.g., different dependency set for that build), initialization will fail at runtime. - The direct package vs bundled
ts_eslint.cjsmay not be perfectly behaviorally identical (transitive deps, parser defaults, patching/shims in the bundle). That’s acceptable for stack traces, but it increases the chance of conformance-only divergence.
At minimum, it’d be good to make the failure mode actionable by catching the require() error and rethrowing with a message that explains how to fix the build/deps.
- Maintainability |
apps/oxlint/src-js/plugins/tokens.ts:156-157
The TODO says to switch conformance back to the bundle once parse errors are fixed, but the change rationale in the PR description is to get stack traces for parsing failures. If the long-term goal is improved debug-ability, switching back later would regress stack trace quality.
If the actual long-term plan is “bundle should preserve readable stack traces,” capture that explicitly (e.g., TODO to generate source maps / preserve function names in the bundle), otherwise the TODO is likely to be misleading and get removed/ignored.
Summary of changes
Summary
Conformance snapshots
- Updated
apps/oxlint/conformance/snapshot.mdto reflect improved stack traces coming from@typescript-eslint/typescript-estreeinternals (e.g.createError,convertError,parser.js) instead of the bundledapps/oxlint/dist/ts_eslint.cjsframes.
TS-ESLint token initialization
- Updated
apps/oxlint/src-js/plugins/tokens.tsto conditionallyrequire()either the direct@typescript-eslint/typescript-estreepackage (for conformance builds) or the local./ts_eslint.cjsbundle (for other builds), and then settsEslintParse = mod.parse. - Added comments documenting the conformance-only behavior and a TODO to revert once parse errors are addressed.
There was a problem hiding this comment.
Pull request overview
This PR modifies the conformance build to use the @typescript-eslint/typescript-estree package directly instead of the bundled ts_eslint.cjs file. This change provides detailed stack traces for parsing failures, making it easier to debug issues during conformance testing. The change only affects the conformance build (gated by the CONFORMANCE flag), while release and debug builds continue to use the bundled version as before.
Key Changes
- Modified token initialization to conditionally require either the direct package or the bundle based on the
CONFORMANCEflag - Updated conformance test snapshots to reflect the more detailed stack traces from the unbundled parser
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
apps/oxlint/src-js/plugins/tokens.ts |
Added conditional logic to load @typescript-eslint/typescript-estree directly in conformance builds instead of the bundled ts_eslint.cjs |
apps/oxlint/conformance/snapshot.md |
Updated stack traces in test snapshots to show detailed paths from the unbundled parser instead of minified bundle references |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Merge activity
|
…age directly in conformance build (#16774) In build for conformance tests, use `@typescript-eslint/typescript-estree` package directly instead of the `ts_eslint.cjs`, to get stack traces for parsing failures. Only affects conformance build - release and debug (tests) builds both use `ts_eslint.cjs` bundle, same as before.
graphite-app
bot
force-pushed
the
12-12-test_linter_plugins_use_typescript-eslint_typescript-estree_package_directly_in_conformance_build
branch
from
7b951e8 to
daa3866
Compare
graphite-app
bot
deleted the
12-12-test_linter_plugins_use_typescript-eslint_typescript-estree_package_directly_in_conformance_build
branch
In build for conformance tests, use
@typescript-eslint/typescript-estreepackage directly instead of thets_eslint.cjs, to get stack traces for parsing failures.Only affects conformance build - release and debug (tests) builds both use
ts_eslint.cjsbundle, same as before.