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
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11072/ |
I would suggest a warning being added as a canary. |
} | ||
} else if ( | ||
stmt.isTSTypeAliasDeclaration() || | ||
stmt.isTSDeclareFunction() || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would stmd.node.id.name
bail on export default function() {}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TSDeclareFunction
(declare function f(): type;
) can't be used in an export default
declaration, so it can never be anonymous.
@babel/babel What do you think about the warning? |
Co-Authored-By: Brian Ng <bng412@gmail.com>
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. |
While I would really like to throw an error, it would break compatibility with |
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. |
@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. |
FWIW if somebody stumbles upon this, suffering from an vast amount of warnings, updating Backstory: I switched from
|
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
2 without 1 isn't enough, because we shouldn't remove the export in cases like this:
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).