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(37092): Improve error when default-importing from a module that export *s from a module with a default export #37212

Merged
merged 2 commits into from
Mar 27, 2020

Conversation

a-tarasyuk
Copy link
Contributor

@a-tarasyuk a-tarasyuk commented Mar 4, 2020

Fixes #37092

@a-tarasyuk a-tarasyuk changed the title feat(37092): improve error message about missing default export feat(37092): Improve error when default-importing from a module that export *s from a module with a default export Mar 4, 2020
@a-tarasyuk a-tarasyuk marked this pull request as ready for review March 19, 2020 21:01
@sandersn sandersn added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Mar 25, 2020
@andrewbranch
Copy link
Member

Two thoughts:

  1. This could really stack up in unhelpful ways given the popularity of barrels and how large they can often become. I would maybe just add the related info once, as something like 'export *' does not re-export a default.
  2. It only makes sense to issue this message if one or more of the ExportStar declaration targets actually has a default export.

@a-tarasyuk
Copy link
Contributor Author

I would maybe just add the related info once, as something like 'export *' does not re-export a default

@andrewbranch Thanks for your feedback 👍. Which node do I need to use to add related info?

@andrewbranch
Copy link
Member

I guess I’d attach it to the first one whose target has a default export.

@sheetalkamat
Copy link
Member

@andrewbranch That makes it hard during incremental editing/diagnostics since then order of the checking would guide the diagnostic position ?

@andrewbranch
Copy link
Member

I could be wrong, but I don’t think that will be the case. exportStar.declarations should be filled by the binder in source order, and then you’d try to resolve the module (resolveExternalModuleName) of each one in order... the order of checking might determine whether those resolutions have already happened before, but I don’t see how that would affect the results you see when checking declarations in source order.

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

Nice! I think this looks great.

@DanielRosenwasser DanielRosenwasser merged commit 3c130d1 into microsoft:master Mar 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Improve error when default-importing from a module that export *s from a module with a default export
5 participants