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

8.2.0 regression #884

Closed
nazar-pc opened this issue Sep 11, 2019 · 14 comments · May be fixed by techsysvbo/kubernetes-tutorials#3
Closed

8.2.0 regression #884

nazar-pc opened this issue Sep 11, 2019 · 14 comments · May be fixed by techsysvbo/kubernetes-tutorials#3
Labels

Comments

@nazar-pc
Copy link

Here is my tsconfig.json:

{
  "compilerOptions": {
    "module": "umd",
    "target": "esnext",
    "sourceMap": true,
    "forceConsistentCasingInFileNames": true,
    "noImplicitReturns": true,
    "noImplicitAny": true,
    "skipLibCheck": true,
    "moduleResolution": "node",
    "allowSyntheticDefaultImports": false,
    "declaration": true,
    "sourceRoot": "src",
    "baseUrl": "src",
    "outDir": "dist",
    "strict": true,
    "noUnusedParameters": true,
    "noUnusedLocals": true,
    "lib": [
      "es2015",
      "es2017.object",
      "dom"
    ],
    "typeRoots": [
      "node_modules/@types",
      "types"
    ],
    "plugins": [
      {
        "name": "typescript-tslint-plugin"
      }
    ]
  },
  "typedocOptions": {
    "mode": "modules",
    "out": "docs"
  },
  "include": [
    "src"
  ]
}

Code sample:

const timeout = setTimeout(() => {}, 0);
if (timeout.unref) {
  timeout.unref();
}

With 8.1.1 it works fine, with 8.2.0 and 8.3.0 it fails with:

error TS2339: Property 'unref' does not exist on type 'number'.

Even though project has @types/node installed and tsc -b works fine and tslint in strict mode doesn't complain either.

@blakeembrey
Copy link
Member

Seems like 9691206 then, which itself was an unexpected breaking change in 8.1.0, it can be re-added in 9.0.

@blakeembrey
Copy link
Member

Can you check by running on 8.0.2?

@blakeembrey
Copy link
Member

Personally, this looks more like you added “dom” to the list of types than a change in versions. I’m pretty sure if you remove DOM it’ll work.

@nazar-pc
Copy link
Author

nazar-pc commented Sep 11, 2019

8.0.2 works fine too.

I can't remove DOM because project targets both environments and a lot of other things will fail to compile because of that.

@blakeembrey
Copy link
Member

Can you create a reproduction? The change I pointed out, which is the only change of code between the version you say broke, didn’t exist on 8.0.2 which is why I asked you to test it. If that works, there must be something else going on.

@blakeembrey
Copy link
Member

Closing because it this should be failing. In browsers setTimeout returns a number and you're using the types from DOM (browsers).

@nazar-pc
Copy link
Author

I believe ts-node should be running successfully anything that compiles with TypeScript compiler. That code compiles in strict mode with TypeScript and runs both in Node.js and Browser.

Moreover, explicit check for property narrows down timeout inside of if branch, hence this code if perfectly correct even if used with DOM types and without any Node.js types at all (while I do have @types/node installed in the project).

Please reopen this issue, I will try to create standalone reproducible example in separate repo.

@blakeembrey blakeembrey reopened this Sep 16, 2019
@blakeembrey
Copy link
Member

blakeembrey commented Sep 16, 2019

I'll take a quick look, you are correct. I'd really appreciate any sort of help in the future, usually I'm commenting without having the capacity to fully test. Apologies for closing this without fully understanding.

@blakeembrey
Copy link
Member

@nazar-pc
Copy link
Author

Verified, 8.4.1 works, thanks!

@adjerbetian
Copy link

@blakeembrey This change triggered a big loss in performance when watching files (with mocha for instance). Would there be another way to solve the problem ?

I'm stuck at version 8.4.0 now (opposite of nazar-pc 😅)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants