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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create TypeScript definitions #4399

Closed
vankop opened this issue Nov 6, 2019 · 23 comments · Fixed by #5582
Closed

Create TypeScript definitions #4399

vankop opened this issue Nov 6, 2019 · 23 comments · Fixed by #5582
Labels
status: ready to implement is ready to be worked on by someone type: tests an improvement to testing

Comments

@vankop
Copy link
Member

vankop commented Nov 6, 2019

There is a killer feature for stylelint in TypeScript 3.7 - now it possible to create declaration file from JS https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#--declaration-and---allowjs

Blocked by: #4245

@vankop vankop added the status: blocked is blocked by another issue or pr label Nov 6, 2019
@hudochenkov
Copy link
Member

I don't think we need a separate issue for this. Upgrade would be handled as with any other dependency.

@vankop
Copy link
Member Author

vankop commented Nov 7, 2019

This issue was not about upgrade at first place, It is about creating definitions =)

@hudochenkov
Copy link
Member

Then rename it and explain a little bit more in the first comment :)

@hudochenkov hudochenkov reopened this Nov 7, 2019
@vankop vankop changed the title move to TypeScript 3.7 create TypeScript definitions Nov 7, 2019
@hudochenkov hudochenkov changed the title create TypeScript definitions Create TypeScript definitions Dec 28, 2019
@jeddy3
Copy link
Member

jeddy3 commented Jan 11, 2020

This is neat. It's great to see JSDocs TypeScript getting some love.

@jeddy3
Copy link
Member

jeddy3 commented Jan 12, 2020

With #4245 now closed, I'm removing the '"blocked" label.

@jeddy3 jeddy3 added status: ready to implement is ready to be worked on by someone type: tests an improvement to testing and removed status: blocked is blocked by another issue or pr labels Jan 12, 2020
@hudochenkov
Copy link
Member

Currently it's blocked by #4505

@hudochenkov hudochenkov added the status: blocked is blocked by another issue or pr label Jan 12, 2020
@jeddy3 jeddy3 removed the status: ready to implement is ready to be worked on by someone label Jan 12, 2020
@ybiquitous
Copy link
Member

Currently it's blocked by #4505

We are using TypeScript 3.7 now,

"typescript": "^3.7.5"

and we will be able to use TypeScript 3.8 soon via #4626! 😄

@vankop vankop added help wanted is likely non-trival and help is wanted and removed status: blocked is blocked by another issue or pr labels Mar 3, 2020
@jeddy3 jeddy3 added status: ready to implement is ready to be worked on by someone and removed help wanted is likely non-trival and help is wanted labels Mar 4, 2020
@tyankatsu0105
Copy link
Member

Hi.
I have a question.
I just found types.
But what for?
At least currently, the types have not been released.

@hudochenkov
Copy link
Member

@tyankatsu0105 we use TypeScript to avoid bugs in our code

"lint:types": "tsc",
We don't use TypeScript language, but JSDoc, which is supported by TypeScript type checker.

@tyankatsu0105
Copy link
Member

@hudochenkov
I see. So these types are not for creating stylelint plugin or something with stylelint but for supporting to develop stylelint self, right?

@hudochenkov
Copy link
Member

Right. But we want to expose them in future, so plugin developers could use them. That's why we have this issue.

@tyankatsu0105
Copy link
Member

But we want to expose them in future, so plugin developers could use them

Awesome!!!
I created stylelint plugin with TypeScript.
https://github.com/tyankatsu0105/stylelint-typescript.
I was worried because @types/stylelint seemed like this wasn't already being maintained.

Thanks for the reply @hudochenkov :)

@glen-84
Copy link
Contributor

glen-84 commented Jun 27, 2020

I tried this now, and most of the declaration files appear to be generated, but there are at least two issues:

#1

lib/utils/parseCalcExpression/parser.js:383:13 - error TS9005: Declaration emit for this file requires using private name 'Parser'. An explicit type annotation may unblock declaration emit.

383 var parser = (function () {
~~~

Found 1 error.

This looks like a generated file, so it probably can't be edited.

Possibly related to microsoft/TypeScript#37832 or microsoft/TypeScript#38382.

#2

Multiple declaration files have code like this:

export = _exports;

... with the error: An export assignment cannot be used in a module with other exported elements.

I couldn't find an existing TypeScript issue regarding this error, but other projects have run into this as well it seems.


You can test it yourself by adding the following to tsconfig.json:

        "declaration": true,
	"emitDeclarationOnly": true,
	"outDir": "types-new"

(and optionally removing "noEmit": true, which is not needed)

Then run npx tsc.

jeddy3 pushed a commit that referenced this issue Jul 12, 2020
* Replace 3rd-party type definitions

This change replaces 3rd-party type definitions under the `types/` directory
with the following `@types/*` npm packages.

- [@types/balanced-match](https://www.npmjs.com/package/@types/balanced-match)
- [@types/imurmurhash](https://www.npmjs.com/package/@types/imurmurhash)
- [@types/postcss-safe-parser](https://www.npmjs.com/package/@types/postcss-safe-parser)
- [@types/style-search](https://www.npmjs.com/package/@types/style-search)
- [@types/svg-tags](https://www.npmjs.com/package/@types/svg-tags)
- [@types/write-file-atomic](https://www.npmjs.com/package/@types/write-file-atomic)

Note that this change does **not** replace all type definitions under `types/`.
We should address the remaining types via other pull requests.

Related to #4399

* Replace with `@types/file-entry-cache`
hudochenkov pushed a commit that referenced this issue Jul 23, 2020
This is a part of #4399.

This change installs the type definition `@types/table` of the `table` package.
The package is used in `lib/formatters/stringFormatter.js`:

https://github.com/stylelint/stylelint/blob/cecacb9d4fe3b5bae5f264efa6e60c414a0f4946/lib/formatters/stringFormatter.js#L8

See also:
- https://www.npmjs.com/package/table
- https://www.npmjs.com/package/@types/table
hudochenkov pushed a commit that referenced this issue Jul 23, 2020
This change adds a type definition for the `postcss-syntax` package.
To find the definition easily, this extracts a new file from the `types/postcss/index.d.ts` file.

Note that there is no `@types/postcss-syntax` package.
(see <https://www.npmjs.com/search?q=%40types%2Fpostcss-syntax>)

Here is a main source of the package:
https://github.com/gucong3000/postcss-syntax/blob/v0.36.2/index.js

Also, `postcss-syntax` is used only in `lib/getPostcssResult.js`:

https://github.com/stylelint/stylelint/blob/cecacb9d4fe3b5bae5f264efa6e60c414a0f4946/lib/getPostcssResult.js#L64-L75

This is a part of #4399.
@Phillip9587
Copy link

Any progress on this? It really sucks that there are only outdated types available. I may help if someone describes the current status to me...

@jeddy3
Copy link
Member

jeddy3 commented Mar 21, 2021

I may help if someone describes the current status to me...

The last attempt (#4399 (comment)) to do this was a year ago and some issues were encountered. Things may have improved since then. So, if you've time you're welcome to look into it. It would be amazing to resolve as TypeScript usage has exploded.

@glen-84
Copy link
Contributor

glen-84 commented Mar 21, 2021

Apart from the 2 issues previously mentioned (which still exist – perhaps related to microsoft/TypeScript#41672), there is also an issue in lib/formatters/index.js, where there is a missing argument (x6).

lib/formatters/index.js:6:11 - error TS2554: Expected 1 arguments, but got 0.

6  compact: importLazy(() => require('./compactFormatter'))(),
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  node_modules/import-lazy/index.d.ts:24:5
    24 ): (moduleId: string) => T;
           ~~~~~~~~~~~~~~~~
    An argument for 'moduleId' was not provided.

@Phillip9587
Copy link

@jeddy3 @glen-84 Would migrating Stylelint to Typescript be an option or are there any reasons you haven't done that yet?

@ybiquitous
Copy link
Member

ybiquitous commented Apr 9, 2021

@glen-84 About #4399 (comment), I've opened the issue to import-lazy: sindresorhus/import-lazy#19

But, to fix the TS error, it seems that we could just specify a module ID as below:

-	compact: importLazy(() => require('./compactFormatter'))(),
+	compact: importLazy(() => require('./compactFormatter'))('compactFormatter'),

@glen-84
Copy link
Contributor

glen-84 commented Apr 9, 2021

@ybiquitous Is stylelint even being bundled in the first place?

@ybiquitous
Copy link
Member

@glen-84 Do you mean stylelint provides a bundle version for browsers?
If so, the answer perhaps is “no” (see also #4796).

I think this lazy loading is for performance, as I see #4278.

@glen-84
Copy link
Contributor

glen-84 commented Apr 10, 2021

@glen-84 Do you mean stylelint provides a bundle version for browsers?

Yes. It looks like @jeddy3 changed the code here.

It could probably be reverted to the standard usage of import-lazy until a decision is made WRT bundling, but there are other issues with the TypeScript build anyway.

@marcfallows
Copy link

Given types exist in https://github.com/stylelint/stylelint/tree/master/types/stylelint, couldn't these be exposed in the files in package.json so consumers of the npm package can use them?

@ybiquitous
Copy link
Member

I've requested to remove the @types/stylelint package: DefinitelyTyped/DefinitelyTyped#57472

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready to implement is ready to be worked on by someone type: tests an improvement to testing
Development

Successfully merging a pull request may close this issue.

8 participants