chore: added type checking using comments #2107

anikethsaha · 2019-07-07T11:32:43Z

For Bugs and Features; did you add new tests?

Added script to do type linting in build

Motivation / Use-Case

As this package as quite a more number of utility methods and logics spread. Type checking is quite needful here. Migrating whole to typescript, for now, is quite a work.
So as discussed with @evilebottnawi , it's better to do the type checking in comments as webpack does too.

Breaking Changes

No
Just an addition of tsconfig.json in root and lint:type in CI

Additional Info

This type-checking is done with reference to jsDoc

Addition dependency :

typescript as devDependency

As of now, I have refactored addEntries.js, and for time being, only included lib/utils/addEntries.js in the tsconfig. as the completing of the changing to dir/files, the tsconfig will be changed.

These changes have no effect on the main logic or the core of the codebase.

codecov · 2019-07-07T11:43:24Z

Codecov Report

Merging #2107 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #2107   +/-   ##
=======================================
  Coverage   92.56%   92.56%           
=======================================
  Files          33       33           
  Lines        1265     1265           
  Branches      361      361           
=======================================
  Hits         1171     1171           
- Misses         87       89    +2     
+ Partials        7        5    -2

Impacted Files	Coverage Δ
lib/utils/addEntries.js	`100% <100%> (ø)`	⬆️
client-src/clients/WebsocketClient.js	`57.14% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c5024c...e34c2fc. Read the comment docs.

hiroppy · 2019-07-07T11:49:40Z

In my option, maintenance of jsdoc is difficult and the possibility of information becoming old becomes high. If you want to introduce types, I think you should move to typescript. If the migration is hard, I will do it. Anyway, I'm -1 on this changes.

anikethsaha · 2019-07-07T11:57:33Z

I am not aiming to create a doc with this, just as type checking in lint! that too not for all directories, just those utilities!
Yeah, typescript migration is more efficient for sure but this can fill the time gap and also it will be 100% with typescript too!

hiroppy · 2019-07-07T12:03:29Z

We've already had next branch, so we can move to typescript in this branch. If you want a type, I will be able to build the base. I think that comments are incomplete and do not make sense.

hiroppy · 2019-07-07T12:27:01Z

@evilebottnawi I wanna know what you think. Honestly, I wanna migrate to typescript, so if you agree with me, I'll migrate to typescript on next branch.

#1984 (comment)

alexander-akait · 2019-07-08T10:11:42Z

/cc @hiroppy better use JSDoc for typescript, it is allow to start migration right now, without major release (we already have many issue for major release, migration on typescriptse increase time to next major release), also it is allow to other users don't know typescript and still contributing webpack-dev-server (we can add types in other PRs), also webpack use same strategies for types so i think it is good idea use types using JSDoc

hiroppy · 2019-07-08T12:31:36Z

sorry but I can't agree with these changes. First, if we want to migrate to typescript, please show me the roadmap. Secand, jsdoc is a bad idea, typescript has allowJS option, so we can migrate to typescript slowly. Also, learning jsdoc costs as same as learning typescript. Sorry but I’m sure this is technical debt. If you use allowJS option, I’ll agree with you. Anyway, first, we must decide we want to migrate to typescript or not. Really sorry.

alexander-akait · 2019-07-08T12:57:38Z

@hiroppy

we must decide we want to migrate to typescript or not

I think yes, sometimes i look on code in some places and can't remember possible types and in some places it is help to maintain, i don't want use pure typescript because it can be hard for other developers to send PRs with fixes so i think using types through JSDocs is good idea. It is not hard priority for us bug it is help to us.

If you use allowJS option, I’ll agree with you.

Can you clarify?

anikethsaha · 2019-07-08T13:51:30Z

I think if we migrate with removing the allowjs options, it will be a big task!.
Keeping allowjs and migrating step by step can be done!

For me, JSDocs is an option which we need even after migrating to TS, not just for now, plus it's giving us the advantage of having a type checking in build too. and without any major change!

Having this and then migrating to TS will help a lot with the migration as we would be having the types in the comment
JSDocs is having good docs so it's not quite a bit of load for us too.

I think webpack core package might have faced the same problem of too much load with migration that's why they have this in the codebase

hiroppy · 2019-07-08T14:45:35Z

If we use JSdoc, when will we move to TypeScript? If you do not make it clear, you can not make a migration forever.

why I think we should avoid Jsdoc?

comments are basically stale and often forgotten
types in jsdoc are very weak
types should be inferred as far as possible

typescript is difficult or not?

Many users have already used typescript, and I think typescript is not difficult because we can use any type. Also, it is easy to move to typescript when we can use any type in the first step because TypeScript owns only types, namespaces(not recommended), interfaces and enums(not recommended). Compatible with es2015 and not complicated.

https://www.npmjs.com/package/typescript

my idea

If you want to migrate to typescript step by step, you can change only that file as typescript using allowJS.

finally

I have experience of moving from flow to typescript in a large-scale product. I think there is no need to make the type strong from the beginning. We used allowJS to migrate to TypeScript.

alexander-akait · 2019-07-08T15:04:46Z

If we use JSdoc, when will we move to TypeScript? If you do not make it clear, you can not make a migration forever.

Should we migrate on typescript fully? webpack use JSDoc for type and all fine so we can do this also, also you can find other popular packages what written on js and use ts only as linting, for example https://github.com/sindresorhus/execa

comments are basically stale and often forgotten

It is not hard to track, also we ccan configure typescript report about this, types should help to us and don't increase work time on issues/features so JSDos is ok solution

types in jsdoc are very weak
types should be inferred as far as possible

look at it from the side that this is just another linter for our code, it is not new language or something also, just linter as eslint

typescript is difficult or not?

yep, typescript is not difficult, rewriting can take us time and for novice users who very often uses webpack, it can be difficult, so will be great keep code in js and use types as part of lint process

anikethsaha · 2019-07-08T15:11:04Z

If you want to migrate to typescript step by step, you can change only that file as typescript using allowJS.

agreed 👍

typescript is difficult or not?

yeah, we can use any, but I think a stable release should come with strict on in tsconfig which will throw with any, so again we need to revisit that any to resolve it.

But for time being sure we can use any and allowJs 👍

Then as per my view if the whole need to migrate to TS (even with step by step using `allowJS`) , it should not be included in this release at least.

For me, we can do both and both seem fair to me, I will give more preference to JSDoc -> TS(with JSDoc)

sokra · 2019-07-12T09:52:00Z

Here my 2 cents.

Choose whatever approach you feel comfortable with the most, but agree on one
- I don't care that much, both approaches are already used in the webpack orgs
Both approaches have pro and cons
jsdoc can't get stale as it is checked by tsc on CI
jsdoc can be forgotten when not using strict mode, similar to types in TS
types in jsdoc are as strong as in TS, both using the typescript compiler under the hood. Sometimes you need a @typedef to create valid JSdoc, where you could inline the code in TS
Both TS and JSdoc uses the same type inference: You need types at the same places in TS or JSDoc
JSDoc is a bit more verbose when not adding descriptions and a bit less verbose when adding descriptions (You need JSDoc in TS to add description comments)
JSDoc is a bit more verbose when using generic types: let x = new Set<Node>() vs /** @type {Set<Node>} */ let x = new Set();
You can't use import with JSDoc, while when using TS you could compile it to require easily
With JSDoc you can publish without compile step
- github and npm code are identical
- Users could consume with github dependency, branches and pull requests can be consumed easily
- yarn link is simpler

A technical clarification: JSDoc means "Using tsc --noEmit --allowJs and add type info via JSDoc where needed"

alexander-akait · 2019-07-12T10:30:43Z

/cc @hiroppy

hiroppy · 2019-07-12T13:40:10Z

@sokra Thank you for answering.

As I said,

If we use JSdoc, when will we move to TypeScript? If you do not make it clear, you can not make a migration forever.

First, I hope we must decide on this choice. If we don't migrate to typescript, I agree to jsdoc, but I think it is easy to migrate to typescript using any. If we migrate to typescript, I'll be able to pay the cost of migrating. fortunately, our organization has jsdoc(webpack) and typescript(webpack-cli), so we have both pieces of knowledge. From now on, our code is bigger than now. I think this choice is important. Thanks.

alexander-akait · 2019-07-12T13:46:19Z

I prefer jsdoc, because it is easy for other developers contribute package, also it is allow to developers solve all on js and when if it is accepted, add types without additional time wasting. Also we can add types step by step without rewriting on typescript and do not spend a lot of time on this and the money of our sponsors

anikethsaha · 2019-07-13T07:02:04Z

either of both looks good !...
just got to make a choice here!

alexander-akait · 2019-07-13T13:40:40Z

/cc @hiroppy @Loonride what do you think guys?

knagaitsev · 2019-07-14T04:25:33Z

My thoughts:

JSDoc seems to be a positive change since it can't get stale via CI, and usually PRs are not introducing many new functions
I don't really have enough experience with typescript to argue for one side or another (I will say that I like not having to recompile anything when working on the server here, but that shouldn't be a deciding factor)

hiroppy · 2019-07-14T12:42:23Z

As I said above I prefer to migrate to typescipt rather than using jsdoc. But I respect @evilebottnawi's opinion. Agree with you.

I prefer jsdoc, because it is easy for other developers contribute package, also it is allow to developers solve all on js and when if it is accepted, add types without additional time wasting. Also we can add types step by step without rewriting on typescript and do not spend a lot of time on this and the money of our sponsors

alexander-akait · 2019-07-15T10:34:34Z

/cc @hiroppy @Loonride so we accept types using JSDocs? Also if we faced with some problem we can always add in our todo rewriting on pure typescript, no need rush this for this issue

hiroppy · 2019-07-15T17:01:45Z

@evilebottnawi Yes, I wanna create a tracking issue like this for contributors and us.

anikethsaha · 2019-07-16T14:21:42Z

Can we have a review on this (on those types)??

cc @evilebottnawi @hiroppy

alexander-akait

Looks fine for me /cc @Loonride want to hear your opinion

/cc @hiroppy need look on the PR, thanks

knagaitsev · 2019-07-18T16:13:56Z

@evilebottnawi I agree with what you said here:

I prefer jsdoc, because it is easy for other developers contribute package, also it is allow to developers solve all on js and when if it is accepted, add types without additional time wasting. Also we can add types step by step without rewriting on typescript and do not spend a lot of time on this and the money of our sponsors

JSDoc seems easy enough to add in, and I agree that typescript can be an additional barrier for contributors. This looks good to me.

hiroppy

lgtm

alexander-akait

👍 Don't forget we focus on websockets and CLI and next major release in near future, so types is not high priority right

wizardofhogwarts

fix CI errors. LGTM.

anikethsaha added 2 commits July 7, 2019 16:44

feat: added typechecking using comments

87cd61b

chore: refactored the type lint

7c10df8

anikethsaha requested review from alexander-akait and hiroppy as code owners July 7, 2019 11:32

build: added lock file

81c170c

Merge branch 'master' into type/typechecks-using-jsdocs

e29e3ad

Merge branch 'master' into type/typechecks-using-jsdocs

8714efe

alexander-akait approved these changes Jul 18, 2019

View reviewed changes

hiroppy approved these changes Jul 18, 2019

View reviewed changes

alexander-akait approved these changes Jul 19, 2019

View reviewed changes

Merge branch 'master' into type/typechecks-using-jsdocs

ac82117

wizardofhogwarts approved these changes Jul 21, 2019

View reviewed changes

evilebottnawi added 2 commits July 22, 2019 13:42

Merge branch 'master' into type/typechecks-using-jsdocs

ddf94bd

Merge branch 'master' into type/typechecks-using-jsdocs

e34c2fc

alexander-akait merged commit c511081 into webpack:master Jul 27, 2019

knagaitsev pushed a commit to knagaitsev/webpack-dev-server that referenced this pull request Jul 31, 2019

refactor: added type checking using comments (webpack#2107)

bb493cc

anikethsaha mentioned this pull request Aug 2, 2019

chore: added types for few utility methods #2172

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: added type checking using comments #2107

chore: added type checking using comments #2107

anikethsaha commented Jul 7, 2019

codecov bot commented Jul 7, 2019 •

edited

Loading

hiroppy commented Jul 7, 2019

anikethsaha commented Jul 7, 2019

hiroppy commented Jul 7, 2019

hiroppy commented Jul 7, 2019 •

edited

Loading

alexander-akait commented Jul 8, 2019

hiroppy commented Jul 8, 2019

alexander-akait commented Jul 8, 2019

anikethsaha commented Jul 8, 2019

hiroppy commented Jul 8, 2019 •

edited

Loading

alexander-akait commented Jul 8, 2019

anikethsaha commented Jul 8, 2019

sokra commented Jul 12, 2019

alexander-akait commented Jul 12, 2019

hiroppy commented Jul 12, 2019 •

edited

Loading

alexander-akait commented Jul 12, 2019

anikethsaha commented Jul 13, 2019

alexander-akait commented Jul 13, 2019

knagaitsev commented Jul 14, 2019 •

edited

Loading

hiroppy commented Jul 14, 2019 •

edited

Loading

alexander-akait commented Jul 15, 2019

hiroppy commented Jul 15, 2019

anikethsaha commented Jul 16, 2019

alexander-akait left a comment

knagaitsev commented Jul 18, 2019 •

edited

Loading

hiroppy left a comment

alexander-akait left a comment

wizardofhogwarts left a comment

chore: added type checking using comments #2107

chore: added type checking using comments #2107

Conversation

anikethsaha commented Jul 7, 2019

For Bugs and Features; did you add new tests?

Motivation / Use-Case

Breaking Changes

Additional Info

codecov bot commented Jul 7, 2019 • edited Loading

Codecov Report

hiroppy commented Jul 7, 2019

anikethsaha commented Jul 7, 2019

hiroppy commented Jul 7, 2019

hiroppy commented Jul 7, 2019 • edited Loading

alexander-akait commented Jul 8, 2019

hiroppy commented Jul 8, 2019

alexander-akait commented Jul 8, 2019

anikethsaha commented Jul 8, 2019

hiroppy commented Jul 8, 2019 • edited Loading

why I think we should avoid Jsdoc?

typescript is difficult or not?

my idea

finally

alexander-akait commented Jul 8, 2019

anikethsaha commented Jul 8, 2019

Then as per my view if the whole need to migrate to TS (even with step by step using allowJS) , it should not be included in this release at least.

sokra commented Jul 12, 2019

alexander-akait commented Jul 12, 2019

hiroppy commented Jul 12, 2019 • edited Loading

alexander-akait commented Jul 12, 2019

anikethsaha commented Jul 13, 2019

alexander-akait commented Jul 13, 2019

knagaitsev commented Jul 14, 2019 • edited Loading

hiroppy commented Jul 14, 2019 • edited Loading

alexander-akait commented Jul 15, 2019

hiroppy commented Jul 15, 2019

anikethsaha commented Jul 16, 2019

alexander-akait left a comment

Choose a reason for hiding this comment

knagaitsev commented Jul 18, 2019 • edited Loading

hiroppy left a comment

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

wizardofhogwarts left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 7, 2019 •

edited

Loading

hiroppy commented Jul 7, 2019 •

edited

Loading

hiroppy commented Jul 8, 2019 •

edited

Loading

Then as per my view if the whole need to migrate to TS (even with step by step using `allowJS`) , it should not be included in this release at least.

hiroppy commented Jul 12, 2019 •

edited

Loading

knagaitsev commented Jul 14, 2019 •

edited

Loading

hiroppy commented Jul 14, 2019 •

edited

Loading

knagaitsev commented Jul 18, 2019 •

edited

Loading