The primary change in this PR is the addition of the new.kind property toTSModuleDeclaration 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 added2 commits

February 9, 2023 17:09

feat: add.kind toTSModuleDeclaration

6b01edc

tighten up types

6f884b4

bradzacher added the ASTPRs and Issues about the AST structure label

Feb 9, 2023

Copy link

nx-cloudbot commentedFeb 9, 2023•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commite020917. 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 💌 fromNxCloud.

Copy link

Contributor

typescript-eslintbot commentedFeb 9, 2023

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.

Copy link

netlifybot commentedFeb 9, 2023•
edited
Loading

✅ Deploy Preview fortypescript-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... Use your smartphone camera to open QR code link.

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

Copy link

Contributor

fisker commentedFeb 9, 2023

Love you, man.

Copy link

Contributor

fisker commentedFeb 9, 2023•
edited
Loading

I'm not familiar with TypeScript.

Just a reminder, do you knowModuleExpression?

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

Should we wrap body inProgram? Or is it a "program"?

Copy link

MemberAuthor

bradzacher commentedFeb 9, 2023

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.

Copy link

Contributor

fisker commentedFeb 9, 2023

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

fix accidental introduction of null

b4bf4c2

JoshuaKGoldberg previously approved these changes

Feb 9, 2023

View reviewed changes

Copy link

Member

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! 🚀

packages/typescript-estree/src/convert.ts

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

Copy link

Member

JoshuaKGoldbergFeb 9, 2023

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

MemberAuthor

bradzacherFeb 9, 2023

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 asany (which is whatconverChild 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.

packages/ast-spec/src/declaration/TSModuleDeclaration/spec.ts OutdatedShow resolvedHide resolved

bradzacher dismissedJoshuaKGoldberg’sstale review viae7435fc

February 9, 2023 21:06

Copy link

MemberAuthor

bradzacher commentedFeb 9, 2023

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

bradzacher added3 commits

February 10, 2023 13:04

Merge branch 'main' into 6440-add-kind-to-module-ast

8263172

update for new old snapshots

b85070b

merge

e020917

bradzacher force-pushed the6440-add-kind-to-module-ast branch frome7435fc toe020917Compare

February 10, 2023 02:54

bradzacherenabled auto-merge

February 10, 2023 02:56

bradzacherdisabled auto-merge

February 10, 2023 03:03

Copy link

codecovbot commentedFeb 10, 2023•
edited
Loading

Codecov Report

Merging#6443 (e020917) intomain (24fdfc8) willdecrease coverage by0.06%.
The diff coverage is52.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