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

Turn on TypeScript's strict mode #3330

Merged
merged 3 commits into from Nov 14, 2019

Conversation

@ry
Copy link
Collaborator

ry commented Nov 13, 2019

No description provided.

@bartlomieju

This comment has been minimized.

Copy link
Contributor

bartlomieju commented Nov 13, 2019

@ry isn't the issue about strict option (strict type check), not alwaysStrict (use strict directive)?

Edit: Well, obviously alwaysStrict is used implicitly when using strict but my point stands

@rsp

This comment has been minimized.

Copy link
Contributor

rsp commented Nov 13, 2019

@bartlomieju Yes, I believe is that alwaysStrict parses in the JS strict mode (as opposed to the JS sloppy mode, not as in strict null checks etc.) and emits 'use strict' in the output.

The strict flag enables alwaysStrict and noImplicitAny, noImplicitThis, strictBindCallApply, strictNullChecks, strictFunctionTypes and strictPropertyInitialization.

@ry ry changed the title Turn on TypeScript's alwaysStrict Turn on TypeScript's strict mode Nov 13, 2019
@ry ry force-pushed the ry:always_strict branch from d9f41e2 to 490eca3 Nov 13, 2019
@ry

This comment has been minimized.

Copy link
Collaborator Author

ry commented Nov 13, 2019

Ok fixed.

@ry ry force-pushed the ry:always_strict branch from 490eca3 to 71a9b59 Nov 13, 2019
Copy link
Contributor

kitsonk left a comment

LGTM

@@ -127,6 +127,7 @@ pub fn compile_bundle(

let config_json = serde_json::json!({
"compilerOptions": {
"strict": true,

This comment has been minimized.

Copy link
@kitsonk

kitsonk Nov 13, 2019

Contributor

DOH, I didn't realise that we had eliminated this from here, and that our internal code had drifted so far... 😭 sorry...

@ry

This comment has been minimized.

Copy link
Collaborator Author

ry commented Nov 13, 2019

Unfortunately we're going to have to do this in two parts because we reference external code which is not strict:

> ./target/debug/deno fmt tests/badly_formatted.js
Compile https://deno.land/std@v0.23.0/prettier/main.ts
error TS7006: Parameter 'opts' implicitly has an 'any' type.

► https://deno.land/std@v0.23.0/prettier/main.ts:334:21

334 async function main(opts): Promise<void> {
                        ~~~~

@ry ry force-pushed the ry:always_strict branch from 69fc893 to 7e8877f Nov 14, 2019
@rsp

This comment has been minimized.

Copy link
Contributor

rsp commented Nov 14, 2019

Unfortunately we're going to have to do this in two parts because we reference external code which is not strict

@ry I'd like to help with that. What is the correct way here - patching the std modules or PRs to some upstream repos?

Looking at the copyright notice (Copyright © James Long and contributors) this particular file seems like coming from the prettier project itself but maybe it should me patched directly here.

ry added 2 commits Nov 14, 2019
@ry ry requested a review from piscisaureus Nov 14, 2019
@ry ry merged commit 4902a1c into denoland:master Nov 14, 2019
10 checks passed
10 checks passed
test macOS-latest
Details
test_std macOS-latest
Details
test windows-2019
Details
test_std windows-2019
Details
test ubuntu-16.04
Details
test_debug ubuntu-16.04
Details
test_std ubuntu-16.04
Details
bench ubuntu-16.04
Details
lint ubuntu-16.04
Details
license/cla Contributor License Agreement is signed.
Details
@ry ry deleted the ry:always_strict branch Nov 14, 2019
axetroy added a commit to axetroy/deno that referenced this pull request Nov 15, 2019
fix lint

format code

fix

format rust code

update arg name

refactor: per-worker resource table (denoland#3306)

- removes global `RESOURCE_TABLE` - resource tables are now created per `Worker`
  in `State`
- renames `CliResource` to `StreamResource` and moves all logic related
  to it to `cli/ops/io.rs`
- removes `cli/resources.rs`
- adds `state` argument to `op_read` and `op_write` and consequently adds
  `stateful_minimal_op` to `State`
- IMPORTANT NOTE: workers don't have access to process stdio - this is
  caused by fact that dropping worker would close stdout for process
  (because it's constructed from raw handle, which closes underlying file
  descriptor on drop)

Revert "refactor: per-worker resource table (denoland#3306)"

This patch does not work with the recent bundler changes (denoland#3325).
Unfortunately I didn't merge master before landing this patch. It has
something to do with console.log not working inside the compiler worker.

This reverts commit fd62379.

Loader: support .wasm imports (denoland#3328)

* loader: support .wasm imports

* http_server: true

* Support named exports

* Clippy

add RUST_BACKTRACE to ci

refactor: per-worker resource table, take 2 (denoland#3342)

- removes global `RESOURCE_TABLE` - resource tables are now created per `Worker`
  in `State`
- renames `CliResource` to `StreamResource` and moves all logic related
  to it to `cli/ops/io.rs`
- removes `cli/resources.rs`
- adds `state` argument to `op_read` and `op_write` and consequently adds
  `stateful_minimal_op` to `State`
- IMPORTANT NOTE: workers don't have access to process stdio - this is
  caused by fact that dropping worker would close stdout for process
  (because it's constructed from raw handle, which closes underlying file
  descriptor on drop)

Turn on TS strict mode for deno_typescript (denoland#3330)

improve test

add test output message for debug

try fix

update

update

fix lint

disable test

update

update

debug

debug msg

trigger ci

trigger ci 1

update

update

format code
axetroy added a commit to axetroy/deno that referenced this pull request Nov 15, 2019
improve test

add test output message for debug

try fix

update

update

fix lint

disable test

update

update

debug

debug msg

trigger ci

trigger ci 1

update

update

format code
@rsp

This comment has been minimized.

Copy link
Contributor

rsp commented Nov 15, 2019

@ry I may be confused here but it seems that contrary to the changelog Deno v0.24.0 doesn't compile TS files with the --strict compiler flag. Or am I misunderstanding something here?

I wrote tests for all flags that are turned on by --strict (plus some additional ones) here:

https://github.com/rsp/typescript-strict-tests

and the output for Deno v0.24.0 is:

Should give all errors for supported flags:
TSC loose:	TSC strict:	Deno:
OK		ERROR		OK		(noImplicitAny)
OK		ERROR		ERROR		(noImplicitThis)
OK		ERROR		ERROR		(alwaysStrict)
OK		ERROR		OK		(strictBindCallApply)
OK		ERROR		OK		(strictNullChecks)
OK		ERROR		OK		(strictFunctionTypes)
OK		ERROR		OK		(strictPropertyInitialization)
OK		ERROR		OK		(noFallthroughCasesInSwitch)
OK		ERROR		OK		(noImplicitReturns)
OK		ERROR		OK		(noUnusedLocals)
OK		ERROR		OK		(noUnusedParameters)
Supported by Deno:    noImplicitThis alwaysStrict
Included by --strict: noImplicitAny noImplicitThis alwaysStrict strictBindCallApply strictNullChecks strictFunctionTypes strictPropertyInitialization
TSC Version 3.7.2 Node v12.13.0
deno: 0.24.0 v8: 8.0.192 typescript: 3.7.2

First column means that the test program is OK for TSC without a given flag, second that it gives error with that flag (to make sure that the test works fine) and the third is the result for Deno (OK means that it works like TSC without that flag and ERROR means that it works like with that flag).

Should it behave like this after this PR is merged?

(By the way, maybe those tests will be useful - some minimal programs written to work without strict flags and fails with them: https://github.com/rsp/typescript-strict-tests/tree/master/src)

@kitsonk

This comment has been minimized.

Copy link
Contributor

kitsonk commented Nov 16, 2019

What @ry was mentioning is that it needed to be done in two stages... Because the CI process depending on no-strict code, we had to turn strict back on for all the internals, and clean up the problems in std so we would have a stable version (0.24) where prettier which we lazy load can be used, so that we can then turn on strict for the runtime. It is a bit meta...

@rsp

This comment has been minimized.

Copy link
Contributor

rsp commented Nov 16, 2019

@kitsonk thanks for the clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.