Skip to content

Conversation

@brettz9
Copy link
Contributor

@brettz9 brettz9 commented Oct 27, 2025

Prerequisites checklist

What is the purpose of this pull request?

Provide TypeScript support for espree.

What changes did you make? (Give an overview)

  • applied checkJs/allowJs TypeScript to espree.
  • complete test coverage
  • in lib/espree.js, removes an uncovered and apparently unnecessary branch (previously lines 152-154) where a property was being added to an array.

Related Issues

A renewed approach to #544 which doesn't require a special, fragile build routine or non-standard JSDoc tags.

Is there anything you'd like reviewers to focus on?

The @typedef exports that were of previous concern should not be here since the main file does not use them except for public type exports. Otherwise, we are using @import.

Has some added complexity in redefining Acorn/acorn-jsx because Acorn's TypeScript does not concern all properties of relevance to plugin authors. See acornjs/acorn#1404 .

Currently applies acorn-jsx from my own fork as waiting on acornjs/acorn-jsx#139 .

@github-project-automation github-project-automation bot moved this to Needs Triage in Triage Oct 27, 2025
@brettz9 brettz9 force-pushed the types branch 7 times, most recently from 72bc186 to 7564529 Compare October 29, 2025 05:24
@lumirlumir lumirlumir requested a review from a team October 30, 2025 08:35
@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2025

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Nov 9, 2025
Also:
- test: complete coverage
@fasttime
Copy link
Member

Not stale. This PR needs to be reviewed.

@nzakas
Copy link
Member

nzakas commented Nov 11, 2025

@brettz9 thanks for this. We're in the middle of preparing to release the first prerelease of v10, so we're a bit swamped at the moment. We'll get around to reviewing this once our schedules free up a bit.

@github-actions
Copy link
Contributor

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Nov 21, 2025
@fasttime fasttime removed the Stale label Nov 21, 2025
@fasttime fasttime moved this from Needs Triage to Triaging in Triage Nov 30, 2025
@fasttime
Copy link
Member

I think this is a very good approach to autogenerating the types. We could adopt some of these techniques in other repositories to simplify complexity.

@fasttime fasttime moved this from Triaging to Implementing in Triage Nov 30, 2025
@brettz9
Copy link
Contributor Author

brettz9 commented Nov 30, 2025

I think this is a very good approach to autogenerating the types. We could adopt some of these techniques in other repositories to simplify complexity.

Great... If you end up using this approach, I have a branch ready for eslint-scope as well (though it looks like you might already be working on one, sorry!).

@fasttime
Copy link
Member

I think this is a very good approach to autogenerating the types. We could adopt some of these techniques in other repositories to simplify complexity.

Great... If you end up using this approach, I have a branch ready for eslint-scope as well (though it looks like you might already be working on one, sorry!).

We can definitely look into autogenerating types for eslint-scope as well if we can maintain compatibility with the current types in @types/eslint-scope. I'd suggest doing that in a separate pull request once #709 is merged in order to avoid overlaps.

Copy link
Member

@fasttime fasttime left a comment

Choose a reason for hiding this comment

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

Since espree types are a new feature, what do you think about adding tests for it? We already have type tests for eslint-visitor-keys in packages/eslint-visitor-keys/test-d/index.test-d.ts and we could add similar tests for espree as well. DefinitelyTyped also has type tests for @types/espree which could be a useful reference, if we want to build on their work.

"dependencies": {
"acorn": "^8.15.0",
"acorn-jsx": "^5.3.2",
"acorn-jsx": "brettz9/acorn-jsx#esm",
Copy link
Member

@fasttime fasttime Nov 30, 2025

Choose a reason for hiding this comment

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

I'm not sure if Acorn-JSX is still maintained. The last commit in that repo was three years ago. If the maintainer doesn't respond to acornjs/acorn-jsx#137, perhaps we should start considering a different approach for those types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I can look to add the types within this project by the time this may be deemed ready.

@brettz9
Copy link
Contributor Author

brettz9 commented Dec 1, 2025

Since espree types are a new feature, what do you think about adding tests for it?

Sure.

We already have type tests for eslint-visitor-keys in packages/eslint-visitor-keys/test-d/index.test-d.ts and we could add similar tests for espree as well. DefinitelyTyped also has type tests for @types/espree which could be a useful reference, if we want to build on their work.

That was a helpful pattern to follow. I've added a1d2f7a which adds their tests as well as 138f496 which adds the public exports they were missing. Let me know if you think we need coverage for other types (and which ones).

@fasttime
Copy link
Member

fasttime commented Dec 1, 2025

We already have type tests for eslint-visitor-keys in packages/eslint-visitor-keys/test-d/index.test-d.ts and we could add similar tests for espree as well. DefinitelyTyped also has type tests for @types/espree which could be a useful reference, if we want to build on their work.

That was a helpful pattern to follow. I've added a1d2f7a which adds their tests as well as 138f496 which adds the public exports they were missing. Let me know if you think we need coverage for other types (and which ones).

I think that's good enough for a start.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if it's necessary to generate type definitions for all lib files. From what I can see, dist/espree.d.ts only imports EsprimaToken from token-translator.js and EspreeComment from espree.js. If those two types could be inlined somehow, maybe we could avoid generating and distributing type definitions for the entire lib directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was able to move EspreeComment/EspreeToken into the main export file. And for ESM types, I was able to use outFile in the tsconfig.json to ensure that only a single file was built without lib being added.

However, there appear to be at least two problems here:

  1. The generated types look poorly formed to me. For example, there is an import of "espree.js" without a slash at the beginning. I don't know if this will work in production even though it was produced by tsc without errors.
  2. @arethetypeswrong/cli reports an error now with the CJS types. Using outFile, I am not able to change the file extension to end in d.cts, and targeting the d.ts file it generates leads to errors by the lint:types script. With outDir, the lib directory is generated.

I've added the commit so you can see, but I think we should probably revert it unless you know of a way to workaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Implementing

Development

Successfully merging this pull request may close these issues.

3 participants