-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
🤖 User test baselines have changed #26156
Conversation
79cb356
to
15eab95
Compare
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.
@sandersn The remaining changes here look pretty expected based on what you said in the other thread, right?
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.
Let's not merge this yet. There are 3 changes. 2 I don't think are good, and 1 I don't understand.
Exit Code: 1 | ||
Standard output: | ||
node_modules/@types/react-native/index.d.ts(3895,26): error TS2693: 'setImmediate' only refers to a type, but is being used as a value here. | ||
node_modules/@types/react-native/index.d.ts(3896,28): error TS2693: 'clearImmediate' only refers to a type, but is being used as a value here. |
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.
I still don't understand what is going on here.
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.
It seems like this is a good change since it turns out (thanks @weswigham!) that setImmediate/clearImmediate are IE/Edge/Node-only, and @types/node
already includes them. @andy-ms, since you are doing a DefinitelyTyped sweep, do you know whether there are other packages besides react-native that need to be fixed?
@@ -100,15 +100,15 @@ node_modules/lodash/_createFlow.js(56,13): error TS2454: Variable 'wrapper' is u | |||
node_modules/lodash/_createFlow.js(57,13): error TS2454: Variable 'wrapper' is used before being assigned. | |||
node_modules/lodash/_createFlow.js(57,21): error TS2339: Property 'thru' does not exist on type 'LodashWrapper'. | |||
node_modules/lodash/_createFlow.js(65,24): error TS2339: Property 'plant' does not exist on type 'LodashWrapper'. | |||
node_modules/lodash/_createHybrid.js(44,49): error TS2345: Argument of type 'string | Function' is not assignable to parameter of type 'Function'. | |||
node_modules/lodash/_createHybrid.js(44,49): error TS2345: Argument of type 'TimerHandler' is not assignable to parameter of type '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.
This alias is too general and should still be removed.
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.
Filed issue microsoft/TypeScript-DOM-lib-generator#559
@@ -30,6 +30,7 @@ node_modules/npm/bin/npm-cli.js(132,17): error TS2339: Property 'config' does no | |||
node_modules/npm/bin/npm-cli.js(134,17): error TS2339: Property 'config' does not exist on type 'typeof EventEmitter'. | |||
node_modules/npm/bin/npm-cli.js(136,17): error TS2339: Property 'config' does not exist on type 'typeof EventEmitter'. | |||
node_modules/npm/html/static/toc.js(3,40): error TS2531: Object is possibly 'null'. | |||
node_modules/npm/html/static/toc.js(28,3): error TS2531: Object is possibly 'null'. |
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.
document.body: HTMLElement | null
. Was it just HTMLElement
before?
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.
Yea.
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.
After discussion with @weswigham and @rbuckton, we are pretty sure that this should still be HTMLElement
. HTMLElement | null
is even more pedantic than making array indexing nullable. I'll file an issue on TSJS-libgenerator to track this fix.
@andy-ms once that fix is in, all the changes you had to make to DefinitelyTyped will be obsolete, sorry. :(
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.
Filed issue microsoft/TypeScript-DOM-lib-generator#558
OK, now that we have some issues to track follow-up, we can go ahead and merge this change. |
Please review the diff and merge if no changes are unexpected.
You can view the build log here.
cc @weswigham @sandersn @mhegazy