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

feat: add .kind to TSModuleDeclaration #6443

Merged
merged 6 commits into from Feb 10, 2023
Merged

Conversation

bradzacher
Copy link
Member

@bradzacher bradzacher commented Feb 9, 2023

PR Checklist

Overview

The primary change in this PR is the addition of the new .kind property to TSModuleDeclaration to help consumers identify what keyword was used to declare the module declaration.

With the addition of this property it also allowed me to provide better union types to clearly define when a module might be missing a body, have a string id, etc.

cc @fisker

@bradzacher bradzacher added the AST PRs and Issues about the AST structure label Feb 9, 2023
@nx-cloud
Copy link

nx-cloud bot commented Feb 9, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit e020917. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 47 targets

Sent with 💌 from NxCloud.

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @bradzacher!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

@netlify
Copy link

netlify bot commented Feb 9, 2023

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit e020917
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/63e5b1d7242c4e00085609e1
😎 Deploy Preview https://deploy-preview-6443--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@fisker
Copy link
Contributor

fisker commented Feb 9, 2023

Love you, man.

@fisker
Copy link
Contributor

fisker commented Feb 9, 2023

I'm not familiar with TypeScript.

Just a reminder, do you know ModuleExpression?

Have you thought about how these two will work once TypeScript support it?

Should we wrap body in Program? Or is it a "program"?

@bradzacher
Copy link
Member Author

Yeah it's a "program".

Semantically there's a few declarations that aren't allowed inside of a TSModuleDeclaration that are allowed within a Program body. At some point we might make them syntax errors.

I assume that a module expression will be the same as a fully-fledged Program with no restrictions.

@fisker
Copy link
Contributor

fisker commented Feb 9, 2023

I think "ModuleExpression" need update anyway estree/estree#238 (comment)

JoshuaKGoldberg
JoshuaKGoldberg previously approved these changes Feb 9, 2023
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Great! 🚀

}
if (id.type !== AST_NODE_TYPES.Identifier) {
throw new Error('`namespace`s must have an Identifier id');
}
Copy link
Member

Choose a reason for hiding this comment

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

I'll just note that #6247 adds an option to throw or not throw on these errors. Potential merge conflict in the making.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah these errors are just there for fun tbh.
I actually could have left it all typed as any (which is what converChild returns). But I added slightly stricter checks just because.

But these constrains are all actually covered by syntax checks in TS itself so they won't ever get hit.

@bradzacher
Copy link
Member Author

Also - this is waiting until your migration PR lands so I don't need to make changes to the old test infra.

@codecov
Copy link

codecov bot commented Feb 10, 2023

Codecov Report

Merging #6443 (e020917) into main (24fdfc8) will decrease coverage by 0.06%.
The diff coverage is 52.94%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6443      +/-   ##
==========================================
- Coverage   90.66%   90.61%   -0.06%     
==========================================
  Files         371      371              
  Lines       12664    12677      +13     
  Branches     3722     3728       +6     
==========================================
+ Hits        11482    11487       +5     
- Misses        845      853       +8     
  Partials      337      337              
Flag Coverage Δ
unittest 90.61% <52.94%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/typescript-estree/src/convert.ts 84.89% <52.94%> (-0.79%) ⬇️

@bradzacher bradzacher merged commit 2f948df into main Feb 10, 2023
@bradzacher bradzacher deleted the 6440-add-kind-to-module-ast branch February 10, 2023 03:05
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
AST PRs and Issues about the AST structure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AST change proposal: Improve TSModuleDeclaration
3 participants