🤖 User test baselines have changed#26186
Conversation
| Standard output: | ||
| node_modules/jimp/jimp.d.ts(268,16): error TS7010: 'appendConstructorOption', which lacks return-type annotation, implicitly has an 'any' return type. | ||
| node_modules/jimp/jimp.d.ts(270,19): error TS2304: Cannot find name 'function'. | ||
| node_modules/jimp/jimp.d.ts(271,18): error TS2304: Cannot find name 'function'. |
There was a problem hiding this comment.
This looks like jimp updated and now has some strictness errors?
| Type 'string' is not assignable to type 'number'. | ||
| lib/FrameManager.js(773,15): error TS2503: Cannot find namespace 'Protocol'. | ||
| lib/NetworkManager.js(82,5): error TS2322: Type '{}' is not assignable to type '{ [x: string]: string; }'. | ||
| Index signature is missing in type '{}'. |
There was a problem hiding this comment.
Right. So this new error is a tad awkward and is cause by the index signature removal. Should an assignment like this be allowed? I looked at this - it looks like so:
const headers = {}
if (a) {
headers["foo"] = "whatever";
}
doThingWithIndexableObject(headers);So there's no intent as far as types go there, but it worked before... I could patch it to work (allow assigning js literals to things with indexes), however the assignment is certainly unsafe. I could have easily done
const headers = {}
if (a) {
headers["foo"] = 42; // Not a string!
}
doThingWithIndexableObject(headers);and have hilarious runtime issues hidden by the any indexer. So I'm on the fence on this one. I kinda wanna just leave it like this, since this is certainly more correct. @sandersn @RyanCavanaugh ?
There was a problem hiding this comment.
This is actually a new bug. I filed #26198 to track it.
| @@ -3346,6 +3344,10 @@ node_modules/chrome-devtools-frontend/front_end/cm/codemirror.js(5791,33): error | |||
| node_modules/chrome-devtools-frontend/front_end/cm/codemirror.js(5794,12): error TS2339: Property 'atomic' does not exist on type 'TextMarker'. | |||
| node_modules/chrome-devtools-frontend/front_end/cm/codemirror.js(5800,12): error TS2339: Property 'parent' does not exist on type 'TextMarker'. | |||
| node_modules/chrome-devtools-frontend/front_end/cm/codemirror.js(5800,27): error TS2339: Property 'parent' does not exist on type 'TextMarker'. | |||
| node_modules/chrome-devtools-frontend/front_end/cm/codemirror.js(5836,20): error TS2339: Property 'line' does not exist on type 'Pos | { from: Pos; to: Pos; }'. | |||
There was a problem hiding this comment.
It is just on Pos however. This is a case where a special declaration has given us a union type for the property, but flow control hasn't rectified the type at this location to just be one or the other. @sandersn weren't you looking at this issue generally recently?
There was a problem hiding this comment.
In any case, I think it's correct to issue these errors (certainly better than the silent any). You can clearly see why we've disallowed the access - it could be one of two things, and we don't know which.
Ah, and this doesn't get a fake access allowed because while the object literal type is in fact a js literal, Pos is not - it's a function psuedoclass type. AFAIK it actually shouldn't have been allowed before; it's certainly an error if you ask about Pos on its own! In TS the find method would need to have overloads on the first argument - 1 and -1 return Pos, while any other number returns {from: Pos, to: Pos}.
There was a problem hiding this comment.
Hooo boy, yeah - the idex signature was masking what i'd call a legitimate bug/footgun here. When you had a union like
{a: number, b: number, [index: string]: any} | {d: string, e: string}we'd allow accesses to d and e (after all, the index means they might exist in both types (of any type), right!?!?) but not a and b. While that was how we behaved before, I think with special assignments this is a big hole to have left open in JS (using an object literal suddenly makes you lose type info on your classes)! I think it definitely should stay as it is now.
Please review the diff and merge if no changes are unexpected.
You can view the build log here.
cc @weswigham @sandersn @mhegazy