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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add TypeScript type checking #6862

Merged
merged 32 commits into from Apr 12, 2018
Merged

Add TypeScript type checking #6862

merged 32 commits into from Apr 12, 2018

Conversation

@mohsen1
Copy link
Contributor

mohsen1 commented Mar 25, 2018

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

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

This comment has been minimized.

Copy link

jsf-clabot commented Mar 25, 2018

CLA assistant check
All committers have signed the CLA.

@mhegazy

This comment has been minimized.

Copy link

mhegazy commented Mar 25, 2018

@mohsen1 the 鈥榑this鈥 error looks like a bug, mind filing this on the TS side.

@montogeek

This comment has been minimized.

Copy link
Member

montogeek commented Mar 25, 2018

This was discussed?

@mohsen1

This comment has been minimized.

Copy link
Contributor Author

mohsen1 commented Mar 25, 2018

@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

This comment has been minimized.

Copy link
Member

sokra commented Mar 26, 2018

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...

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

This comment has been minimized.

Copy link
@sokra

sokra Mar 26, 2018

Member

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

This comment has been minimized.

Copy link
@mohsen1

mohsen1 Mar 27, 2018

Author Contributor

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

@sokra

This comment has been minimized.

Copy link
Member

sokra commented Mar 26, 2018

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

This comment has been minimized.

Copy link
Member

Jessidhia commented Mar 26, 2018

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

This comment has been minimized.

Copy link

marvinhagemeister commented Mar 26, 2018

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

This comment has been minimized.

Copy link
Member

Jessidhia commented Mar 26, 2018

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

This comment has been minimized.

Copy link
Member

sokra commented Mar 26, 2018

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

This comment has been minimized.

Copy link
Contributor

sandersn commented Mar 26, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

mohsen1 commented Mar 26, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

mohsen1 commented Mar 27, 2018

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

@sokra
sokra approved these changes Apr 12, 2018
Copy link
Member

sokra left a comment

I think this is ready now.

Let's see if the CI passes.

@sokra sokra requested a review from ooflorent Apr 12, 2018
@sokra sokra changed the title (WIP) Add TypeScript type checking Add TypeScript type checking Apr 12, 2018
@sokra

This comment has been minimized.

Copy link
Member

sokra commented Apr 12, 2018

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

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

This comment has been minimized.

Copy link
@ooflorent

ooflorent Apr 12, 2018

Member

Can we get rid of the comments?

This comment has been minimized.

Copy link
@Jessidhia

Jessidhia Apr 12, 2018

Member

It's a JSON5 file, but can't use .json5 馃し鈥嶁檧锔

This comment has been minimized.

Copy link
@mohsen1

mohsen1 Apr 12, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@douglascrockford

douglascrockford Apr 14, 2018

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

This comment has been minimized.

Copy link
@bennyn

This comment has been minimized.

Copy link
@pocesar

pocesar Apr 24, 2018

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

This comment has been minimized.

Copy link
@romanzoller

romanzoller Apr 25, 2018

@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 webpack-bot added PR: CI-ok and removed PR: CI-not-ok labels Apr 12, 2018
@webpack-bot

This comment has been minimized.

Copy link
Contributor

webpack-bot commented Apr 12, 2018

Thank you for your pull request! The most important CI builds succeeded, we鈥檒l review the pull request soon.

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

This comment has been minimized.

Copy link
@mohsen1

mohsen1 Apr 12, 2018

Author Contributor

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 comment has been minimized.

Copy link
@mohsen1

mohsen1 Apr 12, 2018

Author Contributor

This should not block merging this pull request though..

@webpack-bot

This comment has been minimized.

Copy link
Contributor

webpack-bot commented Apr 12, 2018

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

This comment has been minimized.

Copy link
Contributor Author

mohsen1 commented Apr 12, 2018

@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

This comment has been minimized.

Copy link
Contributor

sandersn commented Apr 12, 2018

@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

This comment has been minimized.

Copy link
Member

TheLarkInn commented Apr 12, 2018

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

@TheLarkInn TheLarkInn merged commit 10282ea into webpack:master Apr 12, 2018
9 of 11 checks passed
9 of 11 checks passed
codecov/patch/integration 68.42% of diff hit (target 90%)
Details
codecov/project/integration 90.99% (-0.02%) compared to 0e88e8a
Details
codecov/changes/integration No unexpected coverage changes found.
Details
codecov/changes/unit No unexpected coverage changes found.
Details
codecov/patch/unit 36.84% of diff hit (target 0%)
Details
codecov/project/unit 22.88% (target 0%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.004%) to 92.656%
Details
licence/cla Contributor License Agreement is signed.
Details
security/snyk No new issues
Details
@TheLarkInn

This comment has been minimized.

Copy link
Member

TheLarkInn commented Apr 12, 2018

@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

This comment has been minimized.

Copy link
Contributor

johnnyreilly commented Apr 12, 2018

Well done @mohsen1!!!

@m-allanson m-allanson mentioned this pull request Apr 13, 2018
@rodoabad

This comment has been minimized.

Copy link

rodoabad commented Apr 16, 2018

Good job! 鉂わ笍

@davidm-public

This comment was marked as off-topic.

Copy link

davidm-public commented Apr 16, 2018

Great job!
I've been using TS for ages now, with Webpack and Vue, and have often wondered why you've been using Babel, as TS can easily target JS5 !! (and it's written by the great Anders Helsberg).

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