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

Do not trust Scope when removing TypeScript types #10174

Merged
merged 5 commits into from Jul 8, 2019

Conversation

@nicolo-ribaudo
Copy link
Member

commented Jul 7, 2019

Q                       A
Fixed Issues? Fixes #10162, fixes https://github.com/gatsbyjs/gatsby/issues/1542v
Patch: Bug Fix? Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

In v7.5.0, we migrated the TypeScript plugin to use Scope to remove exported types. We assumed that if a node wasn't in the scope, it was a type. While this is in theory true, plugins never did a good job at maintaining this "invariant".
Not only popular community plugins (e.g. gaearon/react-hot-loader#1293), but also our own plugins (e.g. #10172).

While updating those plugins makes the scope more reliable, we can't drop support with older versions since it would be a breaking change.

This PR changes the TypeScript plugin so that it removes an export only if

  1. It isn't registered in the scope
  2. We saw it in a type declaration.

2 without 1 isn't enough, because we shouldn't remove the export in cases like this:

var a = 1;
type a = 2;

export { a };

If others agree that this is the correct solution, I will release v7.5.2 as soon as this is merged.

cc @Wolvereness (author of the initial TS update), @JLHwung (fixed #10172), @theKashey (created gaearon/react-hot-loader#1293).

@babel-bot

This comment has been minimized.

Copy link
Collaborator

commented Jul 7, 2019

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

@Wolvereness

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2019

I would suggest a warning being added as a canary.

}
} else if (
stmt.isTSTypeAliasDeclaration() ||
stmt.isTSDeclareFunction() ||

This comment has been minimized.

Copy link
@JLHwung

JLHwung Jul 7, 2019

Contributor

Would stmd.node.id.name bail on export default function() {}?

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Jul 7, 2019

Author Member

TSDeclareFunction (declare function f(): type;) can't be used in an export default declaration, so it can never be anonymous.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

commented Jul 7, 2019

@babel/babel What do you think about the warning?

nicolo-ribaudo and others added 2 commits Jul 7, 2019
Co-Authored-By: Brian Ng <bng412@gmail.com>
@Andarist

This comment has been minimized.

Copy link
Member

commented Jul 7, 2019

IMHO error would be better than warning. From taking a brief look at it I understand that it's rather unsafe transformation in some cases due to how plugins are implemented. And if it's unsafe it would be better (IMHO) to fail fast and force people to implement correct logic in plugins rather than just warning which may pass unnoticed and break somebody.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

commented Jul 7, 2019

While I would really like to throw an error, it would break compatibility with @babel/plugin-proposal-export-default-from <=7.5, @babel/plugin-proposal-export-default-from <=7.5 and react-hot-loader <4.12.5.
We found these plugin during a weekend because they are either really popular, but it would probably break many other less known plugins.

@JLHwung

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2019

While I would really like to throw an error, it would break compatibility with @babel/plugin-proposal-export-default-from <=7.5, @babel/plugin-proposal-export-default-from <=7.5 and react-hot-loader <4.12.5.

I think we can state in the warning that we may throw error since the next major release. It is recommended for plugin user to fix potential problems. So we can finally get rid of these last resort extra check.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2019

@JLHwung I don't think that we will ever throw an error, since when we would be able to throw an error for every binding which is not registered we would also be able to automatically register them.
I belive that such a long warning is already a big incentive for plugin authors to correctly register them anyway 😆

@nicolo-ribaudo nicolo-ribaudo merged commit f48b47c into babel:master Jul 8, 2019
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 87.57% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nicolo-ribaudo nicolo-ribaudo deleted the nicolo-ribaudo:ts-scope-not-safe branch Jul 8, 2019
@whoisteri

This comment has been minimized.

Copy link

commented Jul 23, 2019

FWIW if somebody stumbles upon this, suffering from an vast amount of warnings, updating @babel/plugin-proposal-export-default-from and react-hot-loader (as per this comment) might solve them for you.

Backstory: I switched from typescript-loader to @babel/preset-typescript recently and I got heaps of warnings about this particular issue whenever I launched the dev server or built my app. Kinda like this:

The exported identifier "_default" is not declared in Babel's scope tracker
as a JavaScript value binding, and "@babel/plugin-transform-typescript"
never encountered it as a TypeScript type declaration.
It will be treated as a JavaScript value.

This problem is likely caused by another plugin injecting
"_default" without registering it in the scope tracker. If you are the author
 of that plugin, please use "scope.registerDeclaration(declarationPath)".
The exported identifier "_default" is not declared in Babel's scope tracker
as a JavaScript value binding, and "@babel/plugin-transform-typescript"
never encountered it as a TypeScript type declaration.
It will be treated as a JavaScript value.

This problem is likely caused by another plugin injecting
"_default" without registering it in the scope tracker. If you are the author
 of that plugin, please use "scope.registerDeclaration(declarationPath)".
The exported identifier "_default" is not declared in Babel's scope tracker
as a JavaScript value binding, and "@babel/plugin-transform-typescript"
never encountered it as a TypeScript type declaration.
It will be treated as a JavaScript value.

This problem is likely caused by another plugin injecting
"_default" without registering it in the scope tracker. If you are the author
 of that plugin, please use "scope.registerDeclaration(declarationPath)".
[... this repeats ~80 more times]
@lock lock bot added the outdated label Oct 22, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants
You can’t perform that action at this time.