Skip to content

Conversation

@hi-ogawa
Copy link
Contributor

@hi-ogawa hi-ogawa commented Mar 20, 2025

spec: https://github.com/typescript-eslint/typescript-eslint/blob/6428670b94de0027189803e5a291858d2e45362d/packages/ast-spec/src/declaration/TSModuleDeclaration/spec.ts#L26-L36

It looks like this is a deprecated property as its redundant. I thought of removing it in https://github.com/oxc-project/acorn-test262, but that means I need to modify 10K files there, so I went with fixing this side.

Copy link
Contributor Author

hi-ogawa commented Mar 20, 2025


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

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.

@github-actions github-actions bot added A-ast Area - AST C-bug Category - Bug labels Mar 20, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Mar 20, 2025

CodSpeed Performance Report

Merging #9907 will create unknown performance changes

Comparing 03-20-fix_ast_extree_fix_tsmoduledeclaration.global_ (7c1cb2d) with 03-17-fix_ast_estree_fix_class.id_ (00642cd)

Summary

🆕 33 new benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
🆕 codegen[checker.ts] N/A 22.6 ms N/A
🆕 codegen_sourcemap[checker.ts] N/A 65.7 ms N/A
🆕 isolated-declarations[vue-id.ts] N/A 57.8 ms N/A
🆕 lexer[RadixUIAdoptionSection.jsx] N/A 20.7 µs N/A
🆕 lexer[antd.js] N/A 24.1 ms N/A
🆕 lexer[cal.com.tsx] N/A 5.7 ms N/A
🆕 lexer[checker.ts] N/A 14.5 ms N/A
🆕 lexer[pdf.mjs] N/A 3.8 ms N/A
🆕 linter[RadixUIAdoptionSection.jsx] N/A 2.7 ms N/A
🆕 linter[cal.com.tsx] N/A 1.2 s N/A
🆕 linter[checker.ts] N/A 3 s N/A
🆕 mangler[antd.js] N/A 15.8 ms N/A
🆕 mangler[react.development.js] N/A 292.2 µs N/A
🆕 mangler[typescript.js] N/A 39.4 ms N/A
🆕 minifier[antd.js] N/A 163.6 ms N/A
🆕 minifier[react.development.js] N/A 1.8 ms N/A
🆕 minifier[typescript.js] N/A 288.3 ms N/A
🆕 estree[checker.ts] N/A 96.8 ms N/A
🆕 parser[RadixUIAdoptionSection.jsx] N/A 90.9 µs N/A
🆕 parser[antd.js] N/A 112.3 ms N/A
... ... ... ... ...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

@hi-ogawa hi-ogawa marked this pull request as ready for review March 20, 2025 06:22
@graphite-app graphite-app bot changed the base branch from 03-17-fix_ast_estree_fix_class.id_ to graphite-base/9907 March 21, 2025 05:47
Copy link
Member

@overlookmotel overlookmotel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the way you've done it here is good. Even if the field is deprecated, it still exists in TS-ESLint AST, and some code may rely on it. So we should match that.

@overlookmotel overlookmotel added the 0-merge Merge with Graphite Merge Queue label Mar 21, 2025
Copy link
Member

overlookmotel commented Mar 21, 2025

Merge activity

  • Mar 21, 1:57 AM EDT: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Mar 21, 1:57 AM EDT: Graphite disabled "merge when ready" on this PR due to: a merge conflict with the target branch; resolve the conflict and try again..
  • Mar 21, 1:57 AM EDT: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Mar 21, 2:23 AM EDT: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.

@overlookmotel overlookmotel added 0-merge Merge with Graphite Merge Queue and removed 0-merge Merge with Graphite Merge Queue labels Mar 21, 2025
@overlookmotel
Copy link
Member

You going to merge this or what, Mr Graphite?

@overlookmotel overlookmotel merged commit db84f12 into graphite-base/9907 Mar 21, 2025
27 checks passed
@overlookmotel overlookmotel deleted the 03-20-fix_ast_extree_fix_tsmoduledeclaration.global_ branch March 21, 2025 06:57
@overlookmotel
Copy link
Member

Graphite got stuck. I've merged manually.

graphite-app bot pushed a commit that referenced this pull request Mar 21, 2025
This is a copy of #9907. For some reason Graphite bungled things up, and that PR got merged into the wrong branch.

spec: https://github.com/typescript-eslint/typescript-eslint/blob/6428670b94de0027189803e5a291858d2e45362d/packages/ast-spec/src/declaration/TSModuleDeclaration/spec.ts#L26-L36

It looks like this is a deprecated property as its redundant. I thought of removing it in https://github.com/oxc-project/acorn-test262, but that means I need to modify 10K files there, so I went with fixing this side.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0-merge Merge with Graphite Merge Queue A-ast Area - AST C-bug Category - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants