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

@ts-expect-error breaking app. #6999

Closed
andreespirela opened this issue Aug 9, 2020 · 11 comments
Closed

@ts-expect-error breaking app. #6999

andreespirela opened this issue Aug 9, 2020 · 11 comments
Labels
bug Something isn't working correctly good first issue Good for newcomers

Comments

@andreespirela
Copy link

andreespirela commented Aug 9, 2020

[Details]
I have a transitive dependency that is using https://deno.land/std@0.64.0/node/_util/_util_promisify.ts which has @ts-expect-error.

image

When using strict=true, it throws the same errors, and also, oak 6.0.1 breaks.
image

This was solved before but now it started happening again in STD with the exact same behavior. This was solved here: #6033 by @kitsonk in #6038 .

[Environment]
deno 1.2.3
v8 8.6.334
typescript 3.9.2

[Additional]
This behavior started happening in std >= 0.63.0 as it is not happening in std 0.62.0

@ry ry added bug Something isn't working correctly good first issue Good for newcomers labels Aug 9, 2020
@JayHelton
Copy link
Contributor

lll grab this issue

@JayHelton
Copy link
Contributor

The fix for this seems to be pretty easy, but Id like to be able to recreate it for I do that.

What are the actual steps to recreate this? I created a file that has https://deno.land/std@0.64.0/node/_util/_util_promisify.ts as a dependency, but did not see any errors on run or bundle commands.

@andreespirela
Copy link
Author

@JayHelton I can’t tell of an specific way to recreate it because it’s happening at Mandarine’s core when upgrading to std 0.64.0. I’ll investigate if I can isolate this in an example.

@nayeemrmn
Copy link
Collaborator

Like last time, it's happening probably because you have a weakened tsconfig which supresses the expected error. The fix is this replacement:

-  // @ts-expect-error TypeScript (as of 3.7) does not support indexing namespaces by symbol
-  const argumentNames = original[kCustomPromisifyArgsSymbol];
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  const argumentNames = original[kCustomPromisifyArgsSymbol as any];

but you'll probably have to do similar things at other @ts-expect-errors. We should just stop using that directive IMO. @kitsonk?

@andreespirela
Copy link
Author

@nayeemrmn That may be it.

Mandarine's tsconfig:

  
{
    "compilerOptions": {
        "strict": false,
        "noImplicitAny": false,
        "noImplicitThis": false,
        "alwaysStrict": false,
        "strictNullChecks": false,
        "strictFunctionTypes": true,
        "strictPropertyInitialization": false,
        "experimentalDecorators": true,
        "emitDecoratorMetadata": true,
        "allowUmdGlobalAccess": false,
    }
}

@kitsonk
Copy link
Contributor

kitsonk commented Aug 11, 2020

We should just stop using that directive IMO.

But we shouldn't replace it with @ts-ignore either though. It is only for the very very very rare instance where we can't coerce the type system any other way, and we should use @ts-expect-error in those cases.

@JayHelton
Copy link
Contributor

JayHelton commented Aug 12, 2020

Like last time, it's happening probably because you have a weakened tsconfig which supresses the expected error. The fix is this replacement:

-  // @ts-expect-error TypeScript (as of 3.7) does not support indexing namespaces by symbol
-  const argumentNames = original[kCustomPromisifyArgsSymbol];
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  const argumentNames = original[kCustomPromisifyArgsSymbol as any];

but you'll probably have to do similar things at other @ts-expect-errors. We should just stop using that directive IMO. @kitsonk?

error: TS7053 [ERROR]: Element implicitly has an 'any' type because expression of type 'any' can't be used to index type 'Function'.

@nayeemrmn @kitsonk
I dont think we can just cast to an any and es ignore it. Typescript still doesnt support indexing by a symbol, or even any.

Im of the opinion that we should just use @ts-ignore for this use case, and using @ts-expect-error just causes issues for those who use typescript, but dont use a strict tsconfig (which is totally fine, imo). Id rather some lines to ignore the error, instead of coercing the type system in an arguably more complicated and hard to read way. Im sure its possible, im just not sure how succinct it would be.

@nayeemrmn
Copy link
Collaborator

Typescript still doesnt support indexing by a symbol, or even any.

@JayHelton Why wouldn't it work for any? I'm pretty sure I tried it before suggesting.

@JayHelton
Copy link
Contributor

Typescript still doesnt support indexing by a symbol, or even any.

@JayHelton Why wouldn't it work for any? I'm pretty sure I tried it before suggesting.

error: TS7053 [ERROR]: Element implicitly has an 'any' type because expression of type 'any' can't be used to index type 'Function'.
  if (original[kCustomPromisifiedSymbol as any]) {
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

here is the code that produces the error:

// eslint-disable-next-line @typescript-eslint/no-explicit-any
  if (original[kCustomPromisifiedSymbol as any]) {

I believe typescript only supports indexing by string and number.

@nayeemrmn
Copy link
Collaborator

Oops, you're right. The comment in the ts-expect-error directive was wrong/outdated about what the error was. Here's the correct casting:

-  // @ts-expect-error TypeScript (as of 3.7) does not support indexing namespaces by symbol
-  if (original[kCustomPromisifiedSymbol]) {
-    // @ts-expect-error TypeScript (as of 3.7) does not support indexing namespaces by symbol
-    const fn = original[kCustomPromisifiedSymbol];
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  if ((original as any)[kCustomPromisifiedSymbol]) {
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    const fn = (original as any)[kCustomPromisifiedSymbol] as Function;

@nayeemrmn
Copy link
Collaborator

Fixed in #7024.

@ry ry closed this as completed Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

5 participants