Skip to content

fix(estree): ExportNamedDeclaration.exportKind should be type for declare export#10387

Closed
leaysgur wants to merge 4 commits intomainfrom
fix/exportkind
Closed

fix(estree): ExportNamedDeclaration.exportKind should be type for declare export#10387
leaysgur wants to merge 4 commits intomainfrom
fix/exportkind

Conversation

@leaysgur
Copy link
Member

Part of #9705

@graphite-app
Copy link
Contributor

graphite-app bot commented Apr 14, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

@github-actions github-actions bot added A-ast Area - AST C-bug Category - Bug labels Apr 14, 2025
Comment on lines +827 to +828
const exportKind = DESER[ImportOrExportKind](POS_OFFSET.export_kind);
THIS.declaration?.declare ? 'type' : exportKind
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
const exportKind = DESER[ImportOrExportKind](POS_OFFSET.export_kind);
THIS.declaration?.declare ? 'type' : exportKind
THIS.declaration?.declare ? 'type' : THIS.exportKind

First I tried this. But this seems to generate invalid JS code as a result. 🤔

@codspeed-hq
Copy link

codspeed-hq bot commented Apr 14, 2025

CodSpeed Instrumentation Performance Report

Merging #10387 will not alter performance

Comparing fix/exportkind (77b0549) with main (2e1ef4c)

Summary

✅ 36 untouched benchmarks

fn serialize<S: Serializer>(&self, serializer: S) {
if let Some(decl) = &self.0.declaration {
if decl.declare() {
ImportOrExportKind::Type.serialize(serializer);
Copy link
Member

Choose a reason for hiding this comment

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

I wonder whether this is a parser bug, where declared exports should be type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the review.

I tried to fix parser at first, but other tests like codegen, isolated_decls failed, so I switched to this one. 😓
I will take another look at the parser side.

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 we need to fix the parser so that consumers of this AST node knows it's a type export.

To test the theory, we can find a linter rule and double check, e.g.

https://github.com/typescript-eslint/typescript-eslint/blob/fa265a970535e9b8d3ac8bfde9f3a24f1a1ec94a/packages/eslint-plugin/src/rules/consistent-type-exports.ts#L205

I haven't checked, but we may be linting this node incorrectly.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I think I got it this time. => #10389

Since it seems that consistent-type-exports is a type-checking rule, so we don't have it yet. 👀
And other rules are not affected by exportKind.

@leaysgur leaysgur marked this pull request as draft April 14, 2025 03:56
@leaysgur leaysgur closed this Apr 14, 2025
@leaysgur leaysgur deleted the fix/exportkind branch April 14, 2025 04:36
Boshen pushed a commit that referenced this pull request Apr 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ast Area - AST C-bug Category - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants