Add TypeScript type checking by mohsen1 · Pull Request #6862 · webpack/webpack

mohsen1 · 2018-03-25T16:38:00Z

Summary

Wanted to try using TypeScript with allowJs and checkJs to see if it can improve code correctness.

Webpack is a good case study for using TypeScript compiler on JavaScript projects. This is still a work in progress and I'm not sure if we can use TypeScript to check for errors without any code modifications.

Issues in TypeScript and DefiantlyTyped

Issues found in Webpack by TypeScript

1. Accessing compilationDependencies in Compilation (Fix: Initialize compilationDependencies in Compilation as undefined #6885)
2. Passing 3 args to WebAssemblyImportDependency Remove extra argument from WebAssemblyImportDependency constructor call site #6882
3. JavascriptGenerator has no constructor but getting arguments Remove extra argument passed to JavascriptGenerator constructor #6881
4. setProperty with too many args Remove extraneous argument from setOptions call sites in OptionsDefaulter #6875 Remove extra argument passed to getEntrypointSize in SizeLimitsPlugin #6900 Remove extra argument sent to HarmonyExportExpressionDependencyConcat… #6899 Remove extra argument passed to ContextModuleFactory in Compiler #6898
5. hideStack is being appended to Error class.
- Fix Add HarmonyLinkingError class #6932
6. Unused dependencyTemplatesHashMap weak map Use a Map for dependencyTemplatesHashMap instead of a WeakMap #6922
7. Using Array methods on Set Use add() to append items to activeExports set in HarmonyExportImporedSpecifierDependency #6950
8. Webpack has some invalid calls to Date and JSON APIs JSON and Date API usage fixes #6982
9. cacheMerged will not work with more than 2 arguments
- TS2448 used before its declaration error for recursive function microsoft/TypeScript#22975
- Simplify cachedMerge #7013 (Thanks to @ooflorent!)

When this is complete?

This is going to be a lot of work to get it to the finish line and I'm not sure if it is inline with the project goals. This is why I'm opening the pull request early. Hoping we can overcome all of those TypeScript issues and make use of TypeScript compiler in Webpack! 🙏

Only comment changes

This pull request on its own should not hav any code changes. TypeScript type checking should be non-invasive and should not make users to change their code. At most, changes should be comments only. If any code change is necessary due to bugs or TS limitations, we are going to propose and merge them in separate pull request.

TypeScript 2.9

The amazing TypeScript team have fixed many issues with the compiler when checking JavaScript code. Those changes are currently (April 11) in the nightly builds of version 2.9. We can merge while depending on nightly version but it would be nicer to depend to beta release in master. We should eventually depend on a stable release.

@TheLarkInn @DanielRosenwasser @mhegazy

jsf-clabot · 2018-03-25T16:38:03Z

All committers have signed the CLA.

mhegazy · 2018-03-25T16:52:42Z

@mohsen1 the ‘_this’ error looks like a bug, mind filing this on the TS side.

montogeek · 2018-03-25T17:39:10Z

This was discussed?

mohsen1 · 2018-03-25T20:56:46Z

@montogeek I don't believe it was discussed before. I'm using Webpack to test the idea of using TypeScript for type checking large JavaScript projects. Would love to finish it up and a complete case study.

Is this is something worth merging if I can complete it? I'm not sure! I would say yes but it's up to owners obviously.

sokra · 2018-03-26T07:49:26Z

This is interesting. I also wanted to try using typescript as linter but had no time for this yet.

Please fix any errors you find in separate PRs so we can merge them independently and this PR doesn't get filled up with fixes. Instead it should only contain the ts-as-linter stuff.

Error 1 seem to be really broken, Error 2 and 3 are also true but have no negative effect...

sokra · 2018-03-26T07:50:09Z

@@ -103,6 +106,7 @@
    "pretest": "npm run lint-files",
    "lint-files": "npm run lint && npm run schema-lint",


call the typescript linting here, to add it to the CI

Will do once number of errors is reduced. The lint bot comments will pollute this thread if I do. See #6869

sokra · 2018-03-26T07:51:37Z

would it be possible to add (stricter) interfaces as additional .d.ts file? I. e. adding Module.d.ts with an interface for the Module.

I only want to know if this is possible. The first PR should only add the infrastructure.

Jessidhia · 2018-03-26T07:58:26Z

It is possible, but the type definitions there are not used to validate the contents of Module.js itself. It only affects other modules that import it.

marvinhagemeister · 2018-03-26T08:32:28Z

One could always add // @ts-check at the top of a ´js` file. See: http://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-3.html

Jessidhia · 2018-03-26T08:38:07Z

You can, it's just that that still doesn't bring any relationship between the .js file and its own .d.ts declaration; you have to write all the JSDoc annotations in duplicate. It's not even possible to refer to types declared on your own .d.ts file unless it's a global type 😢

sokra · 2018-03-26T13:53:30Z

It's sad that it doesn't validate the content ifself, but we can still use it to describe the public interface of these classes.

sandersn · 2018-03-26T16:58:29Z

@mohsen1 can you point me to examples like class Foo {}; let f = new Foo(); f.bar = 1 in the source? I was experimenting with webpack+checkjs a couple of weeks ago and missed this pattern. (You might consider filing on bug on Typescript for it too.)

mohsen1 · 2018-03-26T20:19:56Z

@sokra see #6869

I got lazy and accepted a lot of TypeScript "Quick FIx" suggestions so its not the best change list to fix all issues but there are some interesting things in that PR!

if you're onboard with enabling TS as a linter in Webpack I can take a more incremental approach and make smaller PRs against this branch and go over issues one by one.

mohsen1 · 2018-03-27T03:47:29Z

@sandersn filed a ticket for that issue as well
microsoft/TypeScript#22896

sokra

I think this is ready now.

Let's see if the CI passes.

sokra · 2018-04-12T09:55:52Z

Future PRs could now add types to stuff by using jsdoc. I guess it would be useful to have types on the basic types.

ooflorent · 2018-04-12T09:58:12Z

@@ -0,0 +1,62 @@
+{
+  "compilerOptions": {
+    /* Basic Options */


Can we get rid of the comments?

It's a JSON5 file, but can't use .json5 🤷‍♀️

It's an issue with Github. Commented JSON is fine. Even Mr @douglascrockford is cool with it ;)

I do not know what JSON5 is, but whatever it is, I am not cool with it.

@douglascrockford How about https://hjson.org/? 😄

oh my, way to go on using the ad verecundiam in public @mohsen1

@douglascrockford Re I do not know what JSON5 is:

The JSON5 Data Interchange Format (JSON5) is a superset of JSON that aims to alleviate some of the limitations of JSON by expanding its syntax to include some productions from ECMAScript 5.1.

webpack-bot · 2018-04-12T10:50:11Z

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

mohsen1 · 2018-04-12T15:09:26Z

 				this.errors.push(new ModuleError(this, error));
 			},
 			exec: (code, filename) => {
+				// @ts-ignore Argument of type 'this' is not assignable to parameter of type 'Module'.


normal module is not precisely implementing native module. This is why ts ignore directive is here.

We can use jsdoc @implements to actually resolve it but then this class needs some additional members to satisfy normal module.

This should not block merging this pull request though..

webpack-bot · 2018-04-12T15:13:13Z

It looks like this Pull Request doesn't include enough test cases (based on Code Coverage analysis of the PR diff).

A PR need to be covered by tests if you add a new feature (we want to make sure that your feature is working) or if you fix a bug (we want to make sure that we don't run into a regression in future).

@mohsen1 Please check if this is appliable to your PR and if you can add more test cases.

Read the test readme for details how to write test cases.

mohsen1 · 2018-04-12T16:06:12Z

@sokra Thanks for fixing those issues. Next big task is to add complete JSDoc coverage. Once we have that we can enable the strict mode.

@TheLarkInn We'll need community support to add JSDoc coverage. It would be nice if you can do what you did for converting to ES2015 by involving the community. It's lots of work and we need help!

Lets merge this! 🎉

sandersn · 2018-04-12T16:12:51Z

@mohsen1 note that you can turn on strictNullChecks without any of the other strict flags. In Typescript's test that compiles the npm-distributed version of webpack, we only see 28 "Object is possibly undefined errors".

TheLarkInn · 2018-04-12T17:07:06Z

@mohsen1 thank you so much for this. Let's land it.

TheLarkInn · 2018-04-12T17:20:44Z

@mohsen1 I think it would make sense to create issues that would involve turning each of the strict checks on and implementing the coverage required for them now and contributors can tackle these bit by bit.

johnnyreilly · 2018-04-12T17:38:22Z

Well done @mohsen1!!!

rodoabad · 2018-04-16T13:38:27Z

Good job! ❤️

webpack-bot added PR: unreviewed labels Mar 25, 2018

mohsen1 mentioned this pull request Mar 25, 2018

variable name _this breaks in checkJs microsoft/TypeScript#22866

Closed

mohsen1 mentioned this pull request Mar 25, 2018

Type 'any[]' is not assignable to type '[any, any]' when constructing a Map from array of entries microsoft/TypeScript#22870

Closed

webpack-bot added PR: CI-ok and removed PR: CI-not-ok labels Mar 26, 2018

sokra reviewed Mar 26, 2018

View reviewed changes

mohsen1 mentioned this pull request Mar 26, 2018

Fix type errors after enabling type-checking with TypeScript #6869

Closed

mohsen1 mentioned this pull request Mar 27, 2018

Extending a class method with prototype assignment confuses the compiler about membership type microsoft/TypeScript#22895

Closed

webpack-bot removed the PR: CI-ok label Mar 29, 2018

sokra force-pushed the ts branch from 7f281ae to 9dafa97 Compare April 12, 2018 09:47

remove unneeded member isImportEmitted

a3df74b

sokra approved these changes Apr 12, 2018

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: unreviewed labels Apr 12, 2018

sokra requested a review from ooflorent April 12, 2018 09:53

sokra changed the title ~~(WIP) Add TypeScript type checking~~ Add TypeScript type checking Apr 12, 2018

ooflorent approved these changes Apr 12, 2018

View reviewed changes

webpack-bot added PR: CI-ok and removed PR: CI-not-ok labels Apr 12, 2018

mohsen1 commented Apr 12, 2018

View reviewed changes

TheLarkInn approved these changes Apr 12, 2018

View reviewed changes

TheLarkInn merged commit 10282ea into webpack:master Apr 12, 2018

m-allanson mentioned this pull request Apr 13, 2018

Upgrade flow-bin gatsbyjs/gatsby#2930

Closed

This comment has been minimized.

Sign in to view

renovate Bot mentioned this pull request Apr 17, 2018

Update webpack to 4.8.3-abandoned sbardian/DebtTrackerAPI#21

Closed

azz mentioned this pull request Apr 25, 2018

feat: split out JSON5 prettier/prettier#4367

Merged

azu mentioned this pull request May 16, 2018

Webpack: Use Typescript compiler instead of Babel video-dev/hls.js#1716

Closed

4 of 5 tasks

tschaub mentioned this pull request Jun 22, 2018

TypeScript (Help Wanted) openlayers/openlayers#8120

Closed

reduckted mentioned this pull request Jul 22, 2018

Typescriptify Webpack #3277

Closed

phil-lgr mentioned this pull request Feb 12, 2019

Interest in having TypeScript annotations + TSC Compiler checkJS? apache/pouchdb#7647

Closed

		@@ -103,6 +106,7 @@
		"pretest": "npm run lint-files",
		"lint-files": "npm run lint && npm run schema-lint",

Conversation

mohsen1 commented Mar 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Issues in TypeScript and DefiantlyTyped

Issues found in Webpack by TypeScript

When this is complete?

Only comment changes

TypeScript 2.9

Uh oh!

jsf-clabot commented Mar 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhegazy commented Mar 25, 2018

Uh oh!

montogeek commented Mar 25, 2018

Uh oh!

mohsen1 commented Mar 25, 2018

Uh oh!

sokra commented Mar 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sokra commented Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jessidhia commented Mar 26, 2018

Uh oh!

marvinhagemeister commented Mar 26, 2018

Uh oh!

Jessidhia commented Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sokra commented Mar 26, 2018

Uh oh!

sandersn commented Mar 26, 2018

Uh oh!

mohsen1 commented Mar 26, 2018

Uh oh!

mohsen1 commented Mar 27, 2018

Uh oh!

sokra left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sokra commented Apr 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webpack-bot commented Apr 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webpack-bot commented Apr 12, 2018

Uh oh!

mohsen1 commented Apr 12, 2018

Uh oh!

sandersn commented Apr 12, 2018

Uh oh!

TheLarkInn commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheLarkInn commented Apr 12, 2018

Uh oh!

mohsen1 commented Mar 25, 2018 •

edited

Loading

jsf-clabot commented Mar 25, 2018 •

edited

Loading

sokra commented Mar 26, 2018 •

edited

Loading

Jessidhia commented Mar 26, 2018 •

edited

Loading

sokra left a comment •

edited

Loading

TheLarkInn commented Apr 12, 2018 •

edited

Loading