Minor changes in output from TS handling builtin symbols as unique symbols #50919
Conversation
|
@weswigham Thank you for submitting this PR! This is a live comment which I will keep updated. 2 packages in this PRCode ReviewsThis PR can be merged once it's reviewed. Status
Once every item on this list is checked, I'll ask you for permission to merge and publish the changes. InactiveThis PR has been inactive for 26 days — it is still unreviewed! Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 50919,
"author": "weswigham",
"headCommitOid": "ef4d1fed62516c24fcc3e66eef0b68b447282bc8",
"lastPushDate": "2021-01-29T17:27:03.000Z",
"lastActivityDate": "2021-02-24T10:52:06.000Z",
"maintainerBlessed": true,
"hasMergeConflict": false,
"isFirstContribution": false,
"popularityLevel": "Critical",
"pkgInfo": [
{
"name": "lodash",
"kind": "edit",
"files": [
{
"path": "types/lodash/lodash-tests.ts",
"kind": "test"
}
],
"owners": [
"bczengel",
"chrootsu",
"stepancar",
"aj-r",
"e-cloud",
"thorn0",
"jtmthf",
"DomiR",
"WilliamChelman"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Critical"
},
{
"name": "webidl-conversions",
"kind": "edit",
"files": [
{
"path": "types/webidl-conversions/webidl-conversions-tests.ts",
"kind": "test"
}
],
"owners": [
"ExE-Boss"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Popular"
}
],
"reviews": [
{
"type": "approved",
"reviewer": "ExE-Boss",
"date": "2021-02-24T10:52:06.000Z",
"isMaintainer": false
}
],
"ciResult": "pass"
} |
|
🔔 @bczengel @chrootsu @stepancar @aj-r @e-cloud @thorn0 @jtmthf @DomiR @WilliamChelman @ExE-Boss — please review this PR in the next few days. Be sure to explicitly select |
|
👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings. Let’s review the numbers, shall we? Comparison details 📊
It looks like nothing changed too much. I won’t post performance data again unless it gets worse. |
|
@weswigham can you please make this as draft instead till we are ready on the typescript PR. Thanks. It will help not show up in the DT maintainer todo. |
| conversions.any(options); // $ExpectType Options | ||
| conversions.any(RegExp); // $ExpectType RegExpConstructor | ||
| conversions.any(Symbol.toStringTag); // $ExpectType symbol | ||
| conversions.any(Symbol.toStringTag); // $ExpectType symbol || typeof toStringTag |
There was a problem hiding this comment.
I would expect that the stringified type would be typeof Symbol.toStringTag and not just typeof toStringTag.
There was a problem hiding this comment.
It would be if the test node printer specified an enclosingDeclaration for the node to be generated.
|
It has been more than two weeks and this PR still has no reviews. I'll bump it to the DT maintainer queue. Thank you for your patience, @weswigham. (Ping @bczengel, @chrootsu, @stepancar, @aj-r, @e-cloud, @thorn0, @jtmthf, @DomiR, @WilliamChelman, @ExE-Boss.) |
|
@sandersn @sheetalkamat The TS change should have hit the nightly yesterday, so we will probably need to merge this soon~ |
|
Also @elibarzilay I don't think @typescript-bot handles the draft state transition well. Looks like it's using the original PR timestamp, rather than the time it last went un-draft for the maintainer queue. |
See microsoft/TypeScript#42543 for more. This probably shouldn't be merged until that is (though, ofc, the
$ExpectTypeassertions are backwards compatible), and, in fact, it should probably be merged with DT master have it's CI rerun after it is (and a new TS nightly is out), for good measure.