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

Improve non-top-level import/export error, especially for JS #47076

Closed
sandersn opened this issue Dec 9, 2021 · 8 comments · Fixed by #47087
Closed

Improve non-top-level import/export error, especially for JS #47076

sandersn opened this issue Dec 9, 2021 · 8 comments · Fixed by #47087
Labels
Bug A bug in TypeScript Domain: Error Messages The issue relates to error messaging Fixed A PR has been merged for this issue Good First Issue Well scoped, documented and has the green light Help Wanted You can do this

Comments

@sandersn
Copy link
Member

sandersn commented Dec 9, 2021

ES imports and exports can only be used at the top level of a module. This is illegal:

function container() {
  import 'fs'
  export { container }
  namespace N { }
}

The current errors for these three statements are vague and, for JS, contain irrelevant terms:

Actual:

(1) "An import declaration can only be used in a namespace or module."
(2) "An export declaration can only be used in a module."
(3) "A namespace declaration is only allowed in a namespace or module."

Expected:

(1) When the node is in a JS file, "An import declaration can only be used at the top level of a module."
Otherwise, "An import declaration can only be used at the top level of a namespace or module."
(2) When the node is in a JS file, "An export declaration can only be used at the top level of a module."
Otherwise, "An export declaration can only be used at the top level of a namespace or module."
(3) "A namespace declaration is only allowed at the top level of a module."

Implementation:
checkGrammarModuleElementContext issues these errors. I don't know whether it's better to make it smarter or just avoid calling it for case (1).

@sandersn sandersn added Bug A bug in TypeScript Good First Issue Well scoped, documented and has the green light Help Wanted You can do this labels Dec 9, 2021
@sandersn sandersn added this to the Backlog milestone Dec 9, 2021
@fatcerberus
Copy link

Isn’t export used inside namespaces too? So (2) should probably be “An export declaration can only be used at the top level of module or namespace.”

@vicente-s
Copy link

Hey everyone, I'd like to take on this issue.

@lokicodedaily
Copy link

can I work on this issue?

@sandersn
Copy link
Member Author

@lokicodedaily sure, although you might want to co-ordinate with @vicente-s.

@sandersn
Copy link
Member Author

sandersn commented Jan 11, 2022

@fatcerberus Good point. I updated the Expected section.

@islandryu
Copy link
Contributor

islandryu commented Jan 17, 2022

@sandersn
I tried to fix it in #47087, but did I do something wrong in the PR procedure?
I would be grateful for any feedback.

@sandersn
Copy link
Member Author

@islandryu I'm backlogged on community PRs. I'll review it when I can.

@DanielRosenwasser
Copy link
Member

Thank you @islandryu!

@DanielRosenwasser DanielRosenwasser added Domain: Error Messages The issue relates to error messaging Fixed A PR has been merged for this issue labels Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: Error Messages The issue relates to error messaging Fixed A PR has been merged for this issue Good First Issue Well scoped, documented and has the green light Help Wanted You can do this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants