Skip to content
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

[1.1.0] TS2345: Argument of type 'any' is not assignable to parameter of type 'never' #16

Closed
efstathiosntonas opened this issue May 1, 2023 · 18 comments
Labels
help wanted Extra attention is needed

Comments

@efstathiosntonas
Copy link

efstathiosntonas commented May 1, 2023

After upgrading to 1.1.0 it throws this error:
TS2345: Argument of type 'any' is not assignable to parameter of type 'never'

On previous versions it worked just fine. Casting with as does not fix it.

Screenshot 2023-05-01 at 14 16 15

@WebReflection
Copy link
Member

anyone willing to help? I'm bad at TS and I wished latest changes would do the right thing ... apparently that wasn't the case ... if putting back any fixes this, I am OK 'cause I don't use TS myself so I really ma easy-going here.

@WebReflection WebReflection added the help wanted Extra attention is needed label May 2, 2023
@efstathiosntonas
Copy link
Author

efstathiosntonas commented May 2, 2023

@WebReflection I don't see a reason why not using any since we can pass whatever we want into it, right? It will automagically infer the typings when we later use the result. I'm not a typings expert either, just wondering a bit here.

@WebReflection
Copy link
Member

@andrew-pyle as we introduced a regression here ... do you have any idea what should be done to fix it? I am tempted to add any at the end of the StructuredCloneSupportedTypes list and call it a day, but I am not sure if that invalidates the whole StructuredCloneSupportedTypes purpose.

@andrew-pyle
Copy link
Contributor

Since I created on the Typescript definitions in 1.1.0, I'll chime in.

@efstathiosntonas @WebReflection The Structured Clone Algorithm cannot actually accept any. It accepts most types, but not all. For example, neither Function nor DOM Nodes are cloneable. Furthermore, this module accepts fewer types than the algorithm accepts, per the README.

The type definitions in 1.1.0 were chosen to reflect the limitations of this module & the Structured Clone Algorithm.

That said, the built-in Typescript DOM lib types structuredClone with any, so maybe it's better to be loose than specific with the types for this library.

// lib.dom.d.ts
declare function structuredClone(value: any, options?: StructuredSerializeOptions): any;

@andrew-pyle
Copy link
Contributor

Now about the bug.

@efstathiosntonas Does your tsconfig.json for the project have a lib or target of es2015 or greater? Without that library, the never type appears as the return type of structuredClone().

I'm not the most experienced .d.ts author, so maybe there is a library-side solution to avoid the consumer requirement of lib: es1025 or target: es2015.

This is because the accepted types union includes Set and Map, etc, which aren't defined with the default tsconfig.json settings.

@andrew-pyle
Copy link
Contributor

andrew-pyle commented May 2, 2023

@efstathiosntonas I am actually having trouble reproducing the error today, although I've seen it before. Could you share a minimal example which exhibits the error?

Maybe a link to https://www.typescriptlang.org/play?

@WebReflection
Copy link
Member

That said, the built-in Typescript DOM lib types structuredClone with any, so maybe it's better to be loose than specific with the types for this library.

that kinda seals the deal to me ... as types are useful only for TS users so any less friction in using this library might e desirable?

I kinda like that types in here are spcific for what the module exposes but at the same time I wonder if those types are harder to deal with when the source is actually unknown ... would it make sense to add any at the end? At least there are dozen hints of what's expected but any would pass through as well?

@andrew-pyle
Copy link
Contributor

@WebReflection I agree that creating a more specific type leaves us with more potential for friction in using the package. So I'd be OK with using any. But I think having the types reflect the limitations of the module would be best, if you think it's worth the work.

The drawback for any is that in Typescript, the any obliterates all other type possibilities. any is like a black-hole for types. Every type is assignable to any, so there would be no type hints left if we added any to the union type for Cloneable. Adding any to the end of the union would be the same as saying type Cloneable = any

Screen Shot 2023-05-02 at 12 23 27 PM

@WebReflection
Copy link
Member

that's what I wanted to avoid ... but how would one narrow down the type then? would an export of Cloneable type work so that structuredClone(thingy as Cloneable) would be possible? 🤔

@andrew-pyle
Copy link
Contributor

andrew-pyle commented May 2, 2023

@WebReflection I'm not sure I am following your thought. Sorry!

Are you are thinking of the return type for structuredClone? We woudn't need to sacrifice that. The return type of structuredClone function can be generic, even if the parameter is of type any. The return type will still be the type of the argument at the time of invocation, even with the parameter typed any.

// index.d.ts
export default function <T extends any>(
  any: T,
  options?: {
    transfer: Transferable[]; // Standard
    json: boolean; // Nonstandard
    lossy: boolean; // Nonstandard
  }
): T;

// app.ts
const s = 'this is a test';
const clone: string = structuredClone(s);
// All good. clone is of type string

// Or more complex
type AppType = { id: number; date: Date; data: string }
const appData: AppType = { id: 25, date: new Date(),  data: "data" }
const clone: AppType = structuredClone(appData);
// All good. clone is of type AppType

@WebReflection
Copy link
Member

@andrew-pyle that also seems to work nicely ... doesn't it?

@andrew-pyle
Copy link
Contributor

@WebReflection It would work well, I think. The only issue is that TS would accept arguments which aren't supported by the underlying JS implementation: Blob, File, FileList, etc. from the README.

No red squiggles for the unsupported arguments.

@efstathiosntonas
Copy link
Author

efstathiosntonas commented May 2, 2023

@andrew-pyle it’s an Apollo cache query and the query: document has been generated by graphql-codegen.

Tomorrow I’ll create a codesandbox with a minimal repro.

This is the query just to get an idea:

const messages = client.readQuery({
        query: GetChatChannelMessagesDocument,
        variables: {
          chat_channel_id: channelId
        }
      });

@WebReflection
Copy link
Member

No red squiggles for the unsupported arguments.

honestly I don't care ... the README documentation when the polyfill is used is clear and I can't spend every day here addressing every single person that uses any in the wild ... I think I am going to comment out those guards or just put any at the end and we can revisit later if there's anything we can do ... all objects that are not Blob, File, FileList should be accepted, so instead of allow-listing classes, I'd rather exclude if possible ... is something like this usable in TS?

export type Cloneable = Exclude<any, NotSupportedYet>;

???

@WebReflection
Copy link
Member

OK, for the time being I have rolled back to any because there's no solution and all negations around TS seem to be closed or not resolved or wont fix so until we have a way to say any BUT those-types which is also misleading as generic signature because the polyfill uses the native structuredClone behind the scene and there are no such limitations, I'm happy to Kepp It Simple.

@andrew-pyle
Copy link
Contributor

@WebReflection You are right—I don't think there is a way to have a "not" type.

honestly I don't care ... the README documentation when the polyfill is used is clear and I can't spend every day here addressing every single person that uses any in the wild

Just had this thought: Maybe you'd prefer to spin the types out of your repository entirely, and have them go into the DefinitelyTyped repository for third-party type definitions? The type definitions here are just .d.ts files, so there would be nothing lost with this method. And you don't use TS yourself, @WebReflection.

Type definitions on DefinitelyTyped repository can release independently of this module, so it would reduce your responsibility, too.

What do you think?

@WebReflection
Copy link
Member

WebReflection commented May 3, 2023

I have zero time for this ... already spent more than needed and not a single improvement landed in this project, only regressions, so I'll keep it as is and if anyone wants to submit in that repo I'd be more than happy but that person won't be me because so far all this story gave me only troubles ... 2 issues filed after something that supposed to be better? not my cup of tea, sorry.

the other issue #17

@andrew-pyle
Copy link
Contributor

@WebReflection I just looked at @types/ungap__structured_clone on NPM. Looks like I owe you an apology. There are already third-party types for your module on DefinitelyTyped. 🥴. I should have checked there before contributing the type definitions here.

I can submit a PR removing the .d.ts files and linking to the @types/ungap__structured_clone package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants