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

fix: include transitive type declarations with eslint #339

Merged
merged 5 commits into from
Jul 18, 2024

Conversation

sallustfire
Copy link
Contributor

@sallustfire sallustfire commented Jul 18, 2024

typescript-eslint requires type declarations for imports when evaluating type checked lint rules. These declarations are included as inputs to the eslint action and a regression test under examples/ is included to confirm that types from both npm packages and locally imported sources are resolved.

Changes are visible to end-users: yes

  • Searched for relevant documentation and updated as needed: no
  • Breaking change (forces users to change their own code or config): yes
  • Suggested release notes appear below: no

Test plan

  • New test cases added

Fixes #335

@sallustfire
Copy link
Contributor Author

@alexeagle A couple notes:

  • A very old version of moment is included in the examples, so old that it doesn't even have type declarations. I opted to use dayjs as a placeholder to depend on the types of some npm package. I don't follow the thinking in subdir/ where moment is used, so there may be artifacts or this all can be consolidated.
  • I did get bats to work, but ktlint was consistently failing. It looked like a structural error, so I commented it out, but we should revert that if it works in CI and there is just some hermeticity leak that causes it to fail on my system.
  • There's a large diff in the pnpm lockfile, maybe someone applied a formatter to it? I believe this is the generated pnpm style though.

Copy link
Member

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

That was simple, thanks!

@alexeagle alexeagle merged commit 72f42f2 into aspect-build:main Jul 18, 2024
8 checks passed
@sallustfire
Copy link
Contributor Author

@alexeagle No problem. Thanks for reviewing. It probably can be polished a little bit more e.g. we don't need sources for the deps just the transitive declarations. Stuff like that might help in larger repos?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Include transitive declarations with eslint inputs
2 participants