Don’t create expando declarations on alias symbols #39558
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sandersn
approved these changes
Jul 10, 2020
|
Also maybe other callers of forEachIdentifierInEntityName should not work on aliases either. |
sandersn
added a commit
that referenced
this pull request
Jul 10, 2020
taken from #39558 as soon as it was created
|
You’re right; the check can actually be moved to the top of |
sandersn
approved these changes
Jul 10, 2020
sandersn
added a commit
that referenced
this pull request
Aug 17, 2020
* First attempt at aliases for require * test+initial support for const x=require * 1st round of baseline improvements * 2nd round of baseline updates * support property access after require * check @type tag on require * forbid expando missing namespaces on aliases taken from #39558 as soon as it was created * accept error baselines that are good, actually * Scribbling on d.ts emit code * use getSpecifierForModuleSymbol * hideous hack for module.exports of aliases * Fix module.exports.x --> export list emit * fix isLocalImport predicate * require only creates aliases in JS * re-handle json imports * update fourslash baseline * Cleanup in the checker 1. Simplify alias resolution. 2. Simplify variable-like checking. 3. Make binding skip require calls with type tags -- they fall back to the old require-call code and then check from there. I haven't started on the declaration emit code since I don't know what is going on there nearly as well. * Function for getting module name from require call * First round of cleanup plus a new test Found one missing feature, not sure it's worth adding. * more small cleanup * more cleanup, including lint * use trackSymbol, not serializeTypeForDeclaration * Code review comments, plus remove unneeded code Ad-hoc type reference resolution for `require` isn't needed anymore. * find all refs works * remove old ad-hoc code * make it clear that old behaviour is not that correct * update api baselines * remove outdated comment * PR feedback 1. Fix indentation 2. Add comment for exported JSON emit 3. Add test case for nested-namespace exports. * add a fail-case test (which passes!)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #38218
This is also the antifix of #26912 🤷♂️. #32626 didn’t actually solve #26912; it just stopped reporting errors on it. #38218 shows that when an expando merges with an alias in JS, the alias target is actually unreachable. It looks like going in the other direction and making expandos on aliases actually work would be a bit more effort. I’m not sure anyone is asking for that anyway.