WIP: replace flow with typescript-jsdoc #4325

vankop · 2019-10-04T17:52:55Z

Which issue, if any, is this issue related to?

Help wanted 🤗

github-actions · 2019-10-04T18:00:53Z

Coverage remained the same at 96.541% when pulling a8dc55f on to-typescript into 859288d on master.

hudochenkov · 2019-10-04T20:20:38Z

I see you removed flow directive from all files. You decided to remove all Flow code first, and add JSDoc whenever you can think of? Or Flow is still running?

Could you add TypeScript to npm scripts, please? Can it also be added to pretest npm script so we run TypeScript and Flow together?

vankop · 2019-10-04T20:38:52Z

Flow is running, but does not lint anything, I did it just for development purpose (don’t think about flow types). We can revert this commit and commit that removes flow-typed later

hudochenkov · 2019-10-04T21:28:11Z

Looks like our actual JavaScript got broken in the last commit.

vankop · 2019-10-04T21:42:24Z

Looks like our actual JavaScript got broken in the last commit.

yes you are correct =) I will revert this for now

This reverts commit 8b31acf

vankop · 2019-10-04T22:15:42Z

If someone wants to help I suggest to write what you are taking here.

I will fix lib/assignDisabledRanges.js
and take

lib/standalone.js
lib/writeOutputFile.js

I think files from this list are prioritize

vankop · 2019-10-05T06:07:18Z

I have created issue related to ~/lib/assignDisabledRanges #4328

G-Rath · 2019-10-05T21:48:54Z

FYI I've created a PR for adding definitions for globjoin. They should be merged & published within the week :)

vankop · 2019-10-05T21:51:23Z

@G-Rath I know how it works =) I just did not realizes that so many packages support both ways. Thanks for deep explanation! (babel as well provides util for both exports _interopRequireDefault)

vankop · 2019-10-05T22:06:55Z

@G-Rath As I know there is no a lot of typescript in jsdoc examples + it poor documented (if compare with common Typescript), could you suggest any source of examples / rich information about typescript in jsdoc? as I know there are a lot of tests here - https://github.com/microsoft/TypeScript/tree/master/tests/cases/fourslash. However, it hard to find usage some use cases there (folder is pretty large)

G-Rath · 2019-10-05T22:23:55Z

@vankop sadly not really; you've pretty much summed up the state of the situation.

The two places I'd recommend are looking at tsdoc - It's a project so underway it's not been announced, but I stumbled upon it myself a few months ago when looking for info like you. That can give you a good idea of at least how MS/TS would like JS/TSDocs to endup looking - if you just remember to ignore tags that are not official JSDoc ones, I'd expect their examples to already work in TS.

The other place of information is the handbook.

Finally, the eslint team are actually looking into this same thing, and so you might find interesting code in the PR.

I'm also happy to experiment and mess around with things, so you're welcome to chuck code snippets at me to play w/ if something isn't working as nicely as you'd like, or point me at a file you'd like double-checked or think could be improved :)

vankop · 2019-10-06T17:51:35Z

@hudochenkov looks like problem with CI still exists

hudochenkov · 2019-10-06T21:36:16Z

In a totally new places. Might be that it happens with code changes made in this PR. Though locally nothing fails in this branch.

hudochenkov

This PR would be close to impossible to review :(

I've done the following to check not type related changes:

In a separate folder installed npm install @babel/core @babel/cli prettier.
Add config for Prettier from repository and following config for Babel:
```
{
	"generatorOpts": {
		"comments": false
	}
}
```
It removes all comments and doesn't do any transpiling.
Run Babel to remove comments and then Prettier. Run on parent commit files, and then on the latest commit files.
```
npx babel ../stylelint/lib --out-dir before
npx prettier "before/**/*.js" --write
```

Then compare two folders in Kaleidoscope.

There are quite few changes to JavaScript itself.

I've notices some changes and commented on them, but I don't think it makes much sense to do this comparison before this PR is ready and has not failing tests.

hudochenkov · 2019-10-06T21:29:38Z

lib/cli.js

 	}

-	if (cli.flags.version || cli.flags.v) {
+	if (cli.flags.version) {


-h and -v removed. Please, add them back.

as I understand from meow this flags are aliases for help and version so it should work, or there is another behaviour?

hudochenkov · 2019-10-06T21:31:21Z

lib/createStylelintResult.js

 	let source;

-	if (postcssResult) {
+	if (postcssResult && postcssResult.root) {


Why add this check?

Root possible undefined

hudochenkov · 2019-10-06T21:33:28Z

lib/getPostcssResult.js


-	const cached /*: ?postcss$result*/ = stylelint._postcssResultCache.get(options.filePath);
+	const cached = options.filePath ? stylelint._postcssResultCache.get(options.filePath) : undefined;


Why this ternary?

You can’t pass undefined in Map get
method

hudochenkov · 2019-10-06T21:34:03Z

lib/getPostcssResult.js

@@ -58,7 +58,7 @@ module.exports = function(stylelint /*: stylelint$internalApi*/) /*: Promise<?Ob
 						stringify: postcss.stringify,
 					};
 				}
-			} else if (syntax) {
+			} else if (stylelint._options.syntax) {


Why not use syntax as before?

here is a little bit tricky place stylelint._options.syntax is undefined | string so after assigning on line 41 syntax implicitly have type undefined | string, but after we assign to syntax object - {parse: any, stringify: any} as well.

hudochenkov · 2019-10-06T21:34:28Z

lib/getPostcssResult.js

@@ -122,13 +122,23 @@ module.exports = function(stylelint /*: stylelint$internalApi*/) /*: Promise<?Ob
 			return result;
 		})
 		.then((postcssResult) => {
-			stylelint._postcssResultCache.set(options.filePath, postcssResult);
+			if (options.filePath) {


Why additional check?

options.filePath possible undefined same with get method

vankop · 2019-10-07T06:51:32Z

This PR would be close to impossible to review :(

Obviously, it is a problematic to review, if you have an idea how to split this, I will be glad to discuss.

I don't think it makes much sense to do this comparison before this PR is ready

Sorry, I did not have time to fix last 20 warnings. All welcome to take a look on them =)

G-Rath · 2019-10-07T07:12:48Z

if you have an idea how to split this, I will be glad to discuss.

I would recommend focusing on adding the types, rather than removing flow.

For example, there are a lot of files that have been changed b/c you're removed @flow from the top - but there's no reason to do that in this PR, since they're just comments.

Even if flow is generating errors, the whole point of the PR is to move away from flow, so the errors shouldn't matter.

You can also remove the typing files for flow in another PR, as while sure they're not needed anymore, they're also not doing anything and so don't need to be removed in this PR :)

ntwb · 2019-10-07T08:54:36Z

Thanks @G-Rath, and thanks for all your work thus far @vankop, this is all so much out of my wheelhouse, and anything that will help make reviewing this PR easier I'm all for.

So, if we can get away without including the removal of Flow and @flow comments that would be great if possible please.

vankop · 2019-10-07T09:20:59Z

First I can suggest create separate PR with tsconfig and provided types. I think we can take types folder from this PR, then we can make PRs step by step with or without flow (on your demand).

I would recommend focusing on adding the types, rather than removing flow.

I did it for the reason as described above - just for development reasons. As I see now it is not necessary (however some times flow and ts types are misleading)

You can also remove the typing files for flow in another PR, as while sure they're not needed anymore, they're also not doing anything and so don't need to be removed in this PR :)

Yes you are right, we can revert this commit

Even if flow is generating errors, the whole point of the PR is to move away from flow, so the errors shouldn't matter.

I think the main idea to stay with flow while TS will not cover stylelint' "core"

vankop · 2019-10-07T09:23:11Z

@hudochenkov after restarting CI still fail for Node.js 12.x.x, maybe it is relates to my changes =)

ntwb · 2019-10-07T12:29:00Z

after restarting CI still fail for Node.js 12.x.x, maybe it is relates to my changes =)

Was thinking this might be something I. could help with... But,I cannot get the logs of the failing jobs to display past line 29 😞

hudochenkov · 2019-10-07T12:50:34Z

@ntwb it's a scrollable area. I always keep scrollbar enabled in OS settings :)

# Conflicts: # lib/getPostcssResult.js

vankop · 2019-10-11T18:01:49Z

types is fine now, I think I will start to split this in several PRs

G-Rath · 2019-10-13T09:00:49Z

@vankop btw the definitions for globjoin got published :)

I'll circle back during the week and look to create definitions for the other libraries when I have a chance.

Also, table has typings.

vankop · 2019-10-28T10:43:59Z

Thanks all for your reviews! All this work is done in separate issues 🎉

vankop added 4 commits October 4, 2019 20:46

remove flow directive

b148023

add initial tsconfig

e40163e

remove flow-typed folder

7796bf0

add cli.js types

a8dc55f

add types to assignDisabledRanges

8b31acf

move flow stylelint.js types to typescript definitions

7484b02

vankop added 2 commits October 5, 2019 00:43

Revert "add types to assignDisabledRanges"

17d2c3e

This reverts commit 8b31acf

add augmentConfig types

4172e9b

add types to assignDisabledRanges

683ed47

vankop added 14 commits October 5, 2019 09:22

add types to writeOutputFile

7f41238

add types to requireRule, reportUnknownRuleNames

ea77370

add types to createStylelint.js

0fb582b

add types to printConfig

7133fda

add types to normalizeRuleSettings

cb342b3

add types to needlessDisables

052ca10

add types to getConfigForFile

6910353

add types to isPathIgnored

057a46e

add types to dynamicRequire, createPlugin

6493489

fix types for null config for getConfigFile result

ff56837

fix createStylelint types, types for internal api

d1e173b

move DisabledRange definition to stylelint definitions

a90a3f0

define Postcss result, add types to createStylelintResult

10436a6

fix TODOs that depend on postcss and stylelint definitions

6dc55a2

vankop added 6 commits October 6, 2019 13:29

use ignore package types

6137f06

add types for file-entry-cache

458010c

add types to getFormatterOptionsText utils

07dc968

fix types in stringFormatter

697a1f4

add ts-ignore to assignDisabledRanges, blocked by #4328

a283e98

add ts-ignore to createStylelint methods that depend on cosmiconfig

0ed4a0b

hudochenkov reviewed Oct 6, 2019

View reviewed changes

vankop added 3 commits October 11, 2019 20:38

fix: types definitions

9b009b3

Merge branch 'master' into to-typescript

b2d0d1d

# Conflicts: # lib/getPostcssResult.js

fix: types for getPostcssResult

3e9d5bf

Merge branch 'master' into to-typescript

0e2a13b

hudochenkov mentioned this pull request Oct 25, 2019

add types to standalone.js, cli.js, index.js, required utils #4381

Merged

vankop closed this Oct 28, 2019

vankop deleted the to-typescript branch October 28, 2019 10:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: replace flow with typescript-jsdoc #4325

WIP: replace flow with typescript-jsdoc #4325

vankop commented Oct 4, 2019

github-actions bot commented Oct 4, 2019

hudochenkov commented Oct 4, 2019

vankop commented Oct 4, 2019

hudochenkov commented Oct 4, 2019

vankop commented Oct 4, 2019

vankop commented Oct 4, 2019

vankop commented Oct 5, 2019

G-Rath commented Oct 5, 2019

vankop commented Oct 5, 2019 •

edited

vankop commented Oct 5, 2019

G-Rath commented Oct 5, 2019 •

edited

vankop commented Oct 6, 2019

hudochenkov commented Oct 6, 2019

hudochenkov left a comment

hudochenkov Oct 6, 2019

vankop Oct 7, 2019

hudochenkov Oct 6, 2019

vankop Oct 7, 2019

hudochenkov Oct 6, 2019

vankop Oct 7, 2019 •

edited

hudochenkov Oct 6, 2019

vankop Oct 7, 2019

hudochenkov Oct 6, 2019

vankop Oct 7, 2019

vankop commented Oct 7, 2019

G-Rath commented Oct 7, 2019

ntwb commented Oct 7, 2019

vankop commented Oct 7, 2019

vankop commented Oct 7, 2019

ntwb commented Oct 7, 2019 •

edited

hudochenkov commented Oct 7, 2019

vankop commented Oct 11, 2019

G-Rath commented Oct 13, 2019 •

edited

vankop commented Oct 28, 2019


		const cached /: ?postcss$result/ = stylelint._postcssResultCache.get(options.filePath);
		const cached = options.filePath ? stylelint._postcssResultCache.get(options.filePath) : undefined;

WIP: replace flow with typescript-jsdoc #4325

WIP: replace flow with typescript-jsdoc #4325

Conversation

vankop commented Oct 4, 2019

github-actions bot commented Oct 4, 2019

hudochenkov commented Oct 4, 2019

vankop commented Oct 4, 2019

hudochenkov commented Oct 4, 2019

vankop commented Oct 4, 2019

vankop commented Oct 4, 2019

vankop commented Oct 5, 2019

G-Rath commented Oct 5, 2019

vankop commented Oct 5, 2019 • edited

vankop commented Oct 5, 2019

G-Rath commented Oct 5, 2019 • edited

vankop commented Oct 6, 2019

hudochenkov commented Oct 6, 2019

hudochenkov 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

vankop Oct 7, 2019 • edited

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

vankop commented Oct 7, 2019

G-Rath commented Oct 7, 2019

ntwb commented Oct 7, 2019

vankop commented Oct 7, 2019

vankop commented Oct 7, 2019

ntwb commented Oct 7, 2019 • edited

hudochenkov commented Oct 7, 2019

vankop commented Oct 11, 2019

G-Rath commented Oct 13, 2019 • edited

vankop commented Oct 28, 2019

vankop commented Oct 5, 2019 •

edited

G-Rath commented Oct 5, 2019 •

edited

vankop Oct 7, 2019 •

edited

ntwb commented Oct 7, 2019 •

edited

G-Rath commented Oct 13, 2019 •

edited