Enable "strict" mode for runtime code. #3899

maxmellen · 2020-02-06T16:08:04Z

Addresses #3324

This PR picks up where #3426 left off setting TypeScript's strict mode on by default for runtime code and updates the standard library and tests to support those changes.

claassistantio · 2020-02-06T16:08:19Z

All committers have signed the CLA.

maxmellen · 2020-02-07T15:40:27Z

@kitsonk I finally got the CI checks to pass.

Do you think you could have a look at the changes when you get the chance?
I don't mind keeping this branch up to date with upstream changes but I'd like to get an idea if this has any chance of being merged before putting anymore time into it.

kitsonk

A few comments... we should really go out of our way to use @ts-ignore and the not null assertion. It is just "hiding" potential logic errors.

kitsonk · 2020-02-08T02:16:12Z

cli/js/buffer_test.ts

@@ -78,8 +78,8 @@ function repeat(c: string, bytes: number): Uint8Array {

 test(function bufferNewBuffer(): void {
  init();
-  const buf = new Buffer(testBytes.buffer as ArrayBuffer);
-  check(buf, testString);
+  const buf = new Buffer(testBytes!.buffer as ArrayBuffer);


We should use assert(testBytes) here so the not null assertion isn't required.

I only now learned about TypeScript 3.7's "assertion functions" 🙂
I'll update my PR to take advantage of that wherever possible.

kitsonk · 2020-02-08T02:17:03Z

cli/js/chmod_test.ts

@@ -16,7 +16,7 @@ testPerm({ read: true, write: true }, function chmodSyncSuccess(): void {
  // Check success when not on windows
  if (isNotWindows) {
    const fileInfo = Deno.statSync(filename);
-    assertEquals(fileInfo.mode & 0o777, 0o777);
+    assertEquals(fileInfo.mode! & 0o777, 0o777);


And again, we should use assert(fileInfo.mode)

kitsonk · 2020-02-08T02:18:17Z

cli/js/console_test.ts

@@ -14,8 +14,8 @@ const customInspect = Deno.symbols.customInspect;
 const {
  Console,
  stringifyArgs
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any
-} = Deno[Deno.symbols.internal] as any;
+  // @ts-ignore


Was there another reason to change this? We should keep the any.

The issue here is that with the noImplicitAny setting set to true (implicitly turned on by the "strict" flag), TypeScript won't allow indexing Deno with Deno.symbols.internal without adding that symbol as an index in Deno's type declaration.

Another issue there is that I'm not sure that it's possible to add a computed property to a namespace. The only way I could think of to make this work would be to change Deno into a const like in this example. (Note this example will only type check but won't run.)

Do you think it's worth updating the Deno type declaration as described? I'm a bit conflicted because I think it makes sense for Deno to be a namespace and I suppose this internal symbol wouldn't be used by user code. What do you think?

Ok, understand now. Yeah, I think there are potentially ways, but I wouldn't want to hold up this PR. Only minor suggestion would be to put some comment after the @ts-ignore so our future selfs can understand what is going on.

Suggested change

// @ts-ignore

// @ts-ignore typescript doesn't support symbols as indexes

kitsonk · 2020-02-08T02:18:44Z

cli/js/console_test.ts

@@ -306,6 +306,7 @@ test(function consoleTestCallToStringOnLabel(): void {
  for (const method of methods) {
    let hasCalled = false;

+    // @ts-ignore


There should be a way to type it so we don't need the ignore.

Similarly to #3899 (comment), the time of the argument passed to console[method] still wouldn't be correct so I'm not sure how to improve on this.

I think you would have to do something like to remove the @ts-ignore:

const methods: Array<keyof Console> = ["count", "countReset", "time", "timeLog", "timeEnd"];

If I type methods as Array<keyof typeof Console> (apparently Console refers to a value in this context) I then get the following error:

error TS7053: Element implicitly has an 'any' type because expression of type 'string | number | symbol' can't be used to index type 'Console'. No index signature with a parameter of type 'string' was found on type 'Console'. ► file:///Users/max/Developer/deno/cli/js/console_test.ts:310:5 310 console[method]({ ~~~~~~~~~~~~~~~

And even if we got over that error, then we'd have the issue that { toString(): void { hasCalled = true; } } wouldn't be considered a valid argument to these functions anyway (for instance, count only accepts a string), so I think the best thing to do is switch methods back to implicitly be of type string[] as before my changes and keep the @ts-ignore.

🤷‍♂ ok! you win! :-D

kitsonk · 2020-02-08T02:19:06Z

cli/js/console_test.ts

@@ -464,7 +465,7 @@ test(function consoleGroupWarn(): void {
    console.warn("6");
    console.warn("7");
    assertEquals(
-      both.toString(),
+      both!.toString(),


and assert(both) prior to this.

kitsonk · 2020-02-08T02:39:40Z

std/http/server_test.ts

@@ -523,7 +523,7 @@ test(async function testReadRequestError(): Promise<void> {
      assert(test.headers != null);
      assertEquals(err, undefined);
      assertNotEquals(req, Deno.EOF);
-      for (const h of test.headers) {
+      for (const h of test.headers!) {


shouldn't be needed because of the assert???

kitsonk · 2020-02-08T02:42:20Z

std/node/global.ts

@@ -1 +1,2 @@
+// @ts-ignore
 window["global"] = window;


Suggested change

window["global"] = window;

globalThis["global"] = globalThis;

Not your problem, but I didn't realise this was there, also I suspect we need in here:

global { var global: typeof globalThis; }

So that it can be augmented properly without needing to ignore.

kitsonk · 2020-02-08T02:43:02Z

std/node/module.ts

@@ -384,7 +388,7 @@ class Module {

    Module._cache[filename] = module;
    if (parent !== undefined) {
-      relativeResolveCache[relResolveCacheIdentifier] = filename;
+      relativeResolveCache[relResolveCacheIdentifier!] = filename;


assert(relResolveCacheIdentifier) prior to this

kitsonk · 2020-02-08T02:45:14Z

std/path/win32.ts

@@ -331,14 +331,14 @@ export function join(...paths: string[]): string {
  let needsReplace = true;
  let slashCount = 0;
  assert(firstPart != null);
-  if (isPathSeparator(firstPart.charCodeAt(0))) {
+  if (isPathSeparator(firstPart!.charCodeAt(0))) {


the assertion should be working here so the not null assertion isn't needed...

kitsonk · 2020-02-08T02:46:07Z

std/util/deep_assign.ts

@@ -24,7 +24,7 @@ export function deepAssign(
      if (typeof target[key] !== `object` || !target[key]) {
        target[key] = {};
      }
-      deepAssign(target[key] as Record<string, unknown>, value);
+      deepAssign(target[key] as Record<string, unknown>, value!);


assert(value) here or deepAssign should be accepting undefined.

So we should be able to remove the not null assert?

Yep, my bad :)

maxmellen · 2020-02-11T15:49:56Z

@kitsonk I've updated my PR to take in account your comments. I've also raised questions/concerns to some of your comments.
I rebased my branch on master to stay up to date, and to keep things easy to review I've split the bulk of my work in 3 commits:

16ab35c is my work as you initially reviewed it rebased onto master with the git conflicts fixed
f9eccc4 addresses your review comments where I haven't raised questions/concerns of my own
3674746 adds support for strict mode in the code that was added to master since your review

bartlomieju · 2020-02-18T01:10:03Z

@maxmellen awesome work; thank you! I resolved some minor conflict and gonna merge it once CI gets green.

maxmellen · 2020-02-18T11:09:47Z

Hey @bartlomieju!
Just rebased and fixed the latest issues.
My changes since you had a look at my PR are contained in this commit: b499431.
CI passes but I'm not sure why the CLA assistant isn't reporting its status. You should be able to see that I signed it earlier.

bartlomieju · 2020-02-18T13:05:17Z

@kitsonk can you take another look?

kitsonk

Thanks! A few comments, but nothing critical.

kitsonk · 2020-02-18T19:16:31Z

cli/js/console_test.ts

@@ -14,8 +14,8 @@ const customInspect = Deno.symbols.customInspect;
 const {
  Console,
  stringifyArgs
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any
-} = Deno[Deno.symbols.internal] as any;
+  // @ts-ignore


Ok, understand now. Yeah, I think there are potentially ways, but I wouldn't want to hold up this PR. Only minor suggestion would be to put some comment after the @ts-ignore so our future selfs can understand what is going on.

Suggested change

// @ts-ignore

// @ts-ignore typescript doesn't support symbols as indexes

kitsonk · 2020-02-18T19:18:28Z

cli/js/console_test.ts

@@ -306,6 +306,7 @@ test(function consoleTestCallToStringOnLabel(): void {
  for (const method of methods) {
    let hasCalled = false;

+    // @ts-ignore


I think you would have to do something like to remove the @ts-ignore:

const methods: Array<keyof Console> = ["count", "countReset", "time", "timeLog", "timeEnd"];

kitsonk · 2020-02-18T19:20:27Z

cli/js/event_target_test.ts

@@ -4,8 +4,11 @@ import { test, assertEquals } from "./test_util.ts";
 test(function addEventListenerTest(): void {
  const document = new EventTarget();

+  // @ts-ignore


Either way, as long as we put something in there that says something like "tests ignoring the type system for resilience"

kitsonk · 2020-02-18T19:21:11Z

cli/js/event_target_test.ts

-    on(type, callback?, options?): void {
+    on(
+      type: string,
+      callback: (e: Event) => void,


Sorry, callback was optional (bad typing) before the types were added, now it is required.

kitsonk · 2020-02-18T22:00:29Z

cli/js/form_data_test.ts

@@ -36,7 +36,9 @@ test(function formDataParamsGetSuccess(): void {
  formData.append("a", "true");
  formData.append("b", "false");
  formData.append("a", "null");
+  // @ts-ignore


Yeah, we don't want to deviate, so commenting again of testing contra the typings.

kitsonk · 2020-02-18T22:01:07Z

cli/js/form_data_test.ts

@@ -110,6 +114,7 @@ test(function formDataParamsArgumentsCheck(): void {
    let hasThrown = 0;
    let errMsg = "";
    try {
+      // @ts-ignore


Understood.

kitsonk · 2020-02-18T22:01:34Z

cli/js/headers_test.ts

  name1: "value1",
  name2: "value2",
  name3: "value3",
+  // @ts-ignore


Understood.

kitsonk · 2020-02-18T22:03:26Z

cli/js/resources_test.ts

@@ -39,8 +39,8 @@ testPerm({ read: true }, async function resourcesFile(): Promise<void> {
    Object.keys(resourcesAfter).length,
    Object.keys(resourcesBefore).length + 1
  );
-  const newRid = Object.keys(resourcesAfter).find((rid): boolean => {
+  const newRid = +Object.keys(resourcesAfter).find((rid): boolean => {


Yeah, ok... I know Object.keys() doesn't always produce helpful types.

kitsonk · 2020-02-18T22:05:19Z

std/util/deep_assign.ts

@@ -24,7 +24,7 @@ export function deepAssign(
      if (typeof target[key] !== `object` || !target[key]) {
        target[key] = {};
      }
-      deepAssign(target[key] as Record<string, unknown>, value);
+      deepAssign(target[key] as Record<string, unknown>, value!);


So we should be able to remove the not null assert?

Fixes denoland#3324.

Account for code review comments

Post-rebase fixes for strict mode

Account for code review comments

maxmellen · 2020-02-19T17:15:57Z

Hey @bartlomieju, @kitsonk, I rebased my work on master and addressed your comments in my latest commit!

bartlomieju

Thanks @maxmellen - massive work!

ry

LGTM

Fixes denoland/deno#3324 Co-authored-by: Kitson Kelly <me@kitsonkelly.com>

maxmellen mentioned this pull request Feb 6, 2020

Enable "strict" mode for runtime code. #3426

Closed

maxmellen force-pushed the ts-strict branch 7 times, most recently from e620c5d to 7616ec7 Compare February 7, 2020 15:05

maxmellen requested review from ry and bartlomieju February 7, 2020 15:41

kitsonk reviewed Feb 8, 2020

View reviewed changes

maxmellen force-pushed the ts-strict branch 5 times, most recently from cc1f46c to 3674746 Compare February 11, 2020 15:20

maxmellen force-pushed the ts-strict branch from 54f83a7 to b499431 Compare February 18, 2020 10:32

kitsonk reviewed Feb 18, 2020

View reviewed changes

maxmellen and others added 6 commits February 19, 2020 17:16

Update std/manual.md

d91eb58

Enable "strict" mode for runtime code.

d8a7858

Fixes denoland#3324.

Update codebase to support TypeScript strict mode

e98afdf

squash! Update codebase to support TypeScript strict mode

08c3f1f

Account for code review comments

squash! Update codebase to support TypeScript strict mode

f958adf

Post-rebase fixes for strict mode

squash! Update codebase to support TypeScript strict mode

d538cf9

Post-rebase fixes for strict mode

squash! Update codebase to support TypeScript strict mode

a907271

Account for code review comments

maxmellen force-pushed the ts-strict branch from cb51209 to a907271 Compare February 19, 2020 16:16

kitsonk approved these changes Feb 19, 2020

View reviewed changes

bartlomieju approved these changes Feb 19, 2020

View reviewed changes

ry approved these changes Feb 19, 2020

View reviewed changes

ry merged commit 9012556 into denoland:master Feb 19, 2020

keroxp mentioned this pull request Mar 1, 2020

v0.35.0 TSX file can't be compiled #4197

Closed

dev-nicolaos mentioned this pull request Mar 2, 2020

Async Event Handlers are broken in TS files #4220

Closed

caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 21, 2021

Enable TS strict mode by default (denoland/deno#3899)

70928f3

Fixes denoland/deno#3324 Co-authored-by: Kitson Kelly <me@kitsonkelly.com>

caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021

Enable TS strict mode by default (denoland/deno#3899)

f8fd924

Fixes denoland/deno#3324 Co-authored-by: Kitson Kelly <me@kitsonkelly.com>

caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021

Enable TS strict mode by default (denoland/deno#3899)

1a8df60

Fixes denoland/deno#3324 Co-authored-by: Kitson Kelly <me@kitsonkelly.com>

caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021

Enable TS strict mode by default (denoland/deno#3899)

58489cc

Fixes denoland/deno#3324 Co-authored-by: Kitson Kelly <me@kitsonkelly.com>

caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021

Enable TS strict mode by default (denoland/deno#3899)

805312b

Fixes denoland/deno#3324 Co-authored-by: Kitson Kelly <me@kitsonkelly.com>

caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021

Enable TS strict mode by default (denoland/deno#3899)

c2430a5

Fixes denoland/deno#3324 Co-authored-by: Kitson Kelly <me@kitsonkelly.com>

caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021

Enable TS strict mode by default (denoland/deno#3899)

1dafc71

Fixes denoland/deno#3324 Co-authored-by: Kitson Kelly <me@kitsonkelly.com>

caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021

Enable TS strict mode by default (denoland/deno#3899)

e75487a

Fixes denoland/deno#3324 Co-authored-by: Kitson Kelly <me@kitsonkelly.com>

caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Feb 1, 2021

Enable TS strict mode by default (denoland/deno#3899)

623a601

Fixes denoland/deno#3324 Co-authored-by: Kitson Kelly <me@kitsonkelly.com>

	// @ts-ignore
	// @ts-ignore typescript doesn't support symbols as indexes

	window["global"] = window;
	globalThis["global"] = globalThis;

Enable "strict" mode for runtime code. #3899

Enable "strict" mode for runtime code. #3899

Conversation

maxmellen commented Feb 6, 2020 • edited Loading

claassistantio commented Feb 6, 2020 • edited Loading

maxmellen commented Feb 7, 2020

kitsonk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxmellen commented Feb 11, 2020

bartlomieju commented Feb 18, 2020

maxmellen commented Feb 18, 2020

bartlomieju commented Feb 18, 2020

kitsonk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxmellen commented Feb 19, 2020

bartlomieju left a comment

Choose a reason for hiding this comment

ry left a comment

Choose a reason for hiding this comment

maxmellen commented Feb 6, 2020 •

edited

Loading

claassistantio commented Feb 6, 2020 •

edited

Loading