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.

Sign up for GitHub

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

Jump to bottom

Better error messages when Babel fails to parse import = and export =… #7079

Merged
merged 5 commits into from
Dec 21, 2017
Merged

Better error messages when Babel fails to parse import = and export =… #7079

merged 5 commits into from
Dec 21, 2017

Conversation

gs-akhan
Copy link
Contributor

@gs-akhan gs-akhan commented Dec 20, 2017 •
edited
Loading

… syntax from typescript when using babel-plugin-transform-typescript

Q A
Fixed Issues? #7067
Patch: Bug Fix? No
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR
Any Dependency Changes? No
License MIT

The error messages displayed when Typescript's import = and export = were very less descriptive. Giving users no clue about what to do next..
Hence this messages can improve things regarding on how to fix their imports and exports in Typescript.

Detailed discussion can be found at
#7062
#7067

Thanks

Azharuddin added 2 commits December 21, 2017 02:32
Better error messages when Babel fails to parse import = and export =…
5db4e50 
… syntax from typescript when using babel-plugin-transform-typescript
Fixing Test cases
48cde24
@babel-bot
Copy link
Collaborator

babel-bot commented Dec 20, 2017 •
edited
Loading

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/6288/

@nicolo-ribaudo
Copy link
Member

Can you run make fix to format the code?

@gs-akhan
Copy link
Contributor Author

gs-akhan commented Dec 20, 2017 via email

@nicolo-ribaudo
Copy link
Member

Also, it would be better to split the message on multiple lines (lines should be no longer than 80 chars)

e.g.

throw buildCodeframeError(
  "bla bla bla ... .... " +
  "second line to make " +
  "them shorter"
);

@gs-akhan
Copy link
Contributor Author

gs-akhan commented Dec 20, 2017 via email

nicolo-ribaudo
nicolo-ribaudo reviewed Dec 20, 2017
View reviewed changes
packages/babel-plugin-transform-typescript/src/index.js Outdated
@@ -205,11 +205,19 @@ export default function() {
},

TSImportEqualsDeclaration(path) {
throw path.buildCodeFrameError("`import =` is not supported.");
throw path.buildCodeFrameError(
"`import =` is not supported in Babel's Typescript parser " +
Copy link
Member

Choose a reason for hiding this comment

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

I'd use plugin instead of parser, since it is parsed correctly but not transformed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not. Because If you don't use any plugins. It breaks still..

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe is not supported by @babel/plugin-transform-typescript? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

It is not. Because If you don't use any plugins. It breaks still..

If you don' transform it, it works. https://runkit.com/embed/bsijdhpw0mq9

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure either. I wish Babel could parse this to "importdeclaration = Identifier"
I could write codemod to migrate all our legacyJS files to TS. :)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, let's go with is not supported by @babel/plugin-transform-typescript.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.. On its way

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Dec 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I wish Babel could parse this to "importdeclaration = Identifier"
I could write codemod to migrate all our legacyJS files to TS. :)

Just use import identifier from "module" 😛

import = is only kept for backward compatibility, so it is "legacyTS" https:/Microsoft/TypeScript/blob/master/doc/spec.md#1133-import-require-declarations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright Just a grammar review here before I add

throw path.buildCodeFrameError(
          "`import =` is not supported by @babel/plugin-transform-typescript \n" +
            "Please consider using " +
            "`import <moduleName> from '<moduleName>';` alongside " +
            "Typescript's --allowSyntheticDefaultImports option.",
        );

nicolo-ribaudo
nicolo-ribaudo reviewed Dec 20, 2017
View reviewed changes
packages/babel-plugin-transform-typescript/src/index.js Outdated
throw path.buildCodeFrameError("`export =` is not supported.");
throw path.buildCodeFrameError(
"`export =` is not supported in Babel's Typescript parser because " +
"there is no valid ES6 implementation for it.\nPlease consider using `export <value>;`.",
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Dec 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Isn't export = more like export default <value> than export <value>? (I don't use TS very much, so I'm not sure)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Basically you could only export one object earlier. Now support
export default <value> & export <value>

@existentialism existentialism added area: typescript PR: Polish 💅 A type of pull request used for our changelog categories labels Dec 20, 2017
nicolo-ribaudo
nicolo-ribaudo reviewed Dec 20, 2017
View reviewed changes
packages/babel-plugin-transform-typescript/src/index.js Outdated
@@ -205,11 +205,19 @@ export default function() {
},

TSImportEqualsDeclaration(path) {
throw path.buildCodeFrameError("`import =` is not supported.");
throw path.buildCodeFrameError(
"`import =` is not supported by @babel/plugin-transform-typescript \n" +
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Dec 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

You can remove the trailing space (the other message is ok)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicolo-ribaudo it was there earlier too. If I remove it on terminal the words look very closely attached.
screen shot 2017-12-21 at 4 11 16 am

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I meant before \n

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

nicolo-ribaudo
nicolo-ribaudo approved these changes Dec 20, 2017
View reviewed changes
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

👍

@gs-akhan
Copy link
Contributor Author

Yaaaaay.. Cannot express how glad I am.. :)
Looking for many more PRs in future..
Thank you for all your precious time and efforts @nicolo-ribaudo & @existentialism

@nicolo-ribaudo
Copy link
Member

@gs-akhan This just need another approval, than can be merged 🙂

@yavorsky yavorsky merged commit 2190e10 into babel:master Dec 21, 2017
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@existentialism existentialism existentialism approved these changes

@yavorsky yavorsky yavorsky approved these changes

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

Assignees
No one assigned
Labels
area: typescript outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Polish 💅 A type of pull request used for our changelog categories
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gs-akhan @babel-bot @nicolo-ribaudo @existentialism @yavorsky