Dont look for properties of Object and Function type when looking to resolve named import from module with export=#37964
Conversation
|
@typescript-bot test this |
|
Heya @sheetalkamat, I've started to run the extended test suite on this PR at 16b4153. You can monitor the build here. |
|
@typescript-bot user test this |
|
Heya @sheetalkamat, I've started to run the parallelized community code test suite on this PR at 16b4153. You can monitor the build here. |
|
@typescript-bot run dt |
|
Heya @sheetalkamat, I've started to run the parallelized Definitely Typed test suite on this PR at 16b4153. You can monitor the build here. |
…ort=` type to be resolved by named imports Fixes #37165
sheetalkamat
force-pushed
the
importMerge
branch
from
16b4153 to
5af482c
Compare
|
@typescript-bot test this |
|
Heya @sheetalkamat, I've started to run the extended test suite on this PR at 5af482c. You can monitor the build here. |
|
@typescript-bot run dt |
|
Heya @sheetalkamat, I've started to run the parallelized community code test suite on this PR at 5af482c. You can monitor the build here. |
|
Heya @sheetalkamat, I've started to run the parallelized Definitely Typed test suite on this PR at 5af482c. You can monitor the build here. |
|
Heya @sheetalkamat, I've started to run the extended test suite on this PR at 5af482c. You can monitor the build here. |
| // First check if module was specified with "export=". If so, get the member from the resolved type | ||
| if (moduleSymbol && moduleSymbol.exports && moduleSymbol.exports.get(InternalSymbolName.ExportEquals)) { | ||
| symbolFromVariable = getPropertyOfType(getTypeOfSymbol(targetSymbol), name.escapedText); | ||
| symbolFromVariable = getPropertyOfType(getTypeOfSymbol(targetSymbol), name.escapedText, /*skipObjectFunctionPropertyAugment*/ true); |
There was a problem hiding this comment.
Actually, rather than threading through this flag, and worrying about the safety of caching it and have two differently calculated symbols for a property lookup, couldn't we
- Initialize
globalFunctionTypeandglobalObjectTypeeach to a unique empty type (which should naturally cause property lookups that ultimately may have depended on them to fail during global initialization). - Check if
symbolFromVariable.parenthere isglobalObjectTypeorglobalFunctionTypeand, if so, discard the result? (to ensure consistency with the initialization phase)
There was a problem hiding this comment.
- We want this to happen irrespective of augmenting that means any time we import the properties from object/function should be error so this might not happen at augmentation time.
- Will there be some kind of edge case where in valid scenario return globalfunctionType property symbol. That is what if getTypeOfSymbol(targetSymbol) is globalFunctionType or globalObjectType
|
@typescript-bot run dt |
|
Heya @sheetalkamat, I've started to run the parallelized community code test suite on this PR at a252ef9. You can monitor the build here. |
|
Heya @sheetalkamat, I've started to run the extended test suite on this PR at a252ef9. You can monitor the build here. |
|
Heya @sheetalkamat, I've started to run the parallelized Definitely Typed test suite on this PR at a252ef9. You can monitor the build here. |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
@typescript-bot user test this |
|
Heya @sheetalkamat, I've started to run the parallelized community code test suite on this PR at a252ef9. You can monitor the build here. |
|
@typescript-bot run dt |
|
Heya @sheetalkamat, I've started to run the parallelized Definitely Typed test suite on this PR at a252ef9. You can monitor the build here. |
|
Heya @sheetalkamat, I've started to run the extended test suite on this PR at a252ef9. You can monitor the build here. |
|
All the tests passed. |
|
@weswigham looking at your last review, it looks like @sheetalkamat addressed your questions. Is this ready to merge once 4.1 RC is out? |
sheetalkamat
force-pushed
the
importMerge
branch
from
a9e4980 to
5a964bd
Compare
|
Will wait for master to become 4.2 before merging this one. |
4 participants
Fixes #37165