Skip to content

feat: Skip type injection if template uses TypeScript #440

New issue

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

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

Already on GitHub? # to your account

Merged
merged 8 commits into from
Nov 26, 2023

Conversation

baseballyama
Copy link
Member

@baseballyama baseballyama commented Nov 23, 2023

TODO

  • Add various tests.
  • Fix type definition.

Discussion point:

I think somehow we can avoid to generate injection types if there is type information but I couldn't find a way. If we can do this, this is better in terms of performance.

Copy link

changeset-bot bot commented Nov 23, 2023

🦋 Changeset detected

Latest commit: 136e6e0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte-eslint-parser Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@coveralls
Copy link

coveralls commented Nov 23, 2023

Pull Request Test Coverage Report for Build 6965825787

  • 16 of 18 (88.89%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.1%) to 91.443%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/utils/index.ts 10 12 83.33%
Totals Coverage Status
Change from base Build 6953169835: -0.1%
Covered Lines: 2487
Relevant Lines: 2630

💛 - Coveralls

@@ -922,6 +923,9 @@ function buildProcessExpressionForExpression(
typing: string | null,
): (expression: ESTree.Expression) => ScriptLetCallback<ESTree.Expression>[] {
Copy link
Member Author

Choose a reason for hiding this comment

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

@ota-meshi

Should I use TSESTree here? Write ESTree.Expression | TSESTree.Expression everytime is hassle, and since the type information is only needed in a few places, I thought it would be okay to leave the type information as it is. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's OK to leave it as is for now 👍. I think that acorn-typescript nodes and TSESTree are not compatible, so just using TSESTree will not be a complete solution.

@baseballyama baseballyama marked this pull request as ready for review November 23, 2023 11:57
Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

LGTM!

@ota-meshi ota-meshi merged commit 726f21f into main Nov 26, 2023
@ota-meshi ota-meshi deleted the feat/template-ts branch November 26, 2023 23:26
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants