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

[TypeScript] Typing Support #983

Closed
huan opened this issue Jul 7, 2019 · 31 comments
Closed

[TypeScript] Typing Support #983

huan opened this issue Jul 7, 2019 · 31 comments

Comments

@huan
Copy link
Contributor

huan commented Jul 7, 2019

I had spent around two full days to study the document and write the typings for matrix-js-sdk.

With the typing system, it helped me a lot to learn the API and prevent lots of potential bugs. I think it's worth to publish them, the best will be integrated into our npm module.

The typing definition is still working in progress and heavy developing, you can watch it at https://github.com/huan/matrix-appservice-wechaty/blob/master/src/typings/matrix-js-sdk.d.ts.

I'll create a pull request to add the typing to this repository after I get my project done, and any suggestion will be welcome by leaving comments under this issue.

Link to wechaty/matrix-appservice#13

@Half-Shot

Thanks!

@Jack-Works
Copy link
Contributor

Jack-Works commented Nov 19, 2019

any progress on TypeScript declaration file?

I try to use TypeScript 3.7 new feature --declaration with --allowJs to automatically generate the typings from the code base.

I failed with tons of any.

The reason of failure of generating the typing is the matrix code base

  • is mixing the commonjs and ES Modules
  • is using prototype to create a class instead of the class syntax in ES6

image

@huan
Copy link
Contributor Author

huan commented Nov 19, 2019

This new feature from TS 3.7 is really cool.

I'd like to suggest that you try to use my declaration file which is written by me handly from scratch at here: https://github.com/huan/matrix-appservice-wechaty/tree/master/src/typings

And if you think it is useable, I'd like to publish it to the @types/matrix-js-sdk so that we can use it easily, and attract more TS developers to improve it for a better TS integration.

@Jack-Works
Copy link
Contributor

Jack-Works commented Nov 19, 2019

I have tried your declaration file before, but it might be missing some functions(like createClient in the screenshot above) so not working well on our project. I think it is better to generate the types from the project since the project is typed by JSDoc. It will be easier to maintain the types for this huge project.

@huan
Copy link
Contributor Author

huan commented Nov 19, 2019

Yes, It would be great if we can get the declaration from the source code automatically.

I hope you can figure it out, looking forward to the better version of the declaration files! 👍

@Jack-Works
Copy link
Contributor

It need do have some codemods on the current code base like transform all commonjs style import/exports to ESModule, and transform all ES5 class to ES6 class.

@Jack-Works
Copy link
Contributor

And there are lots of JSDoc style type refer like @extends {module:crypto/algorithms/base.EncryptionAlgorithm}, it is needed to parse the JSDoc style to TS style like typeof import("../../crypto/algorithms/base").EncryptionAlgorithm

@turt2live
Copy link
Member

The TypeScript update is that our tests are a disaster and don't work with typescript. We're transitioning to TypeScript proper rather than supplying declarations.

@Jack-Works
Copy link
Contributor

Jack-Works commented Nov 19, 2019

The Babel supports typescript doesn't means it will be typed by TS. I'm going to working on the .d.ts generation from the current codebase and release it as a separate package until the official codebase can provide a high quality typescript definition

Problems when generating dts

  • ES5 style classes
  • mixed module system
  • jsdoc style type refer
  • global type refer

@turt2live
Copy link
Member

@Jack-Works there's a plan to rewrite/port to typescript, and a stage 1 PR is not a final representation of all the work required. I would recommend holding off until we fix our tests so we can get TypeScript properly landed, which will have a stage 2 PR for typings of the stuff we don't port (assuming it doesn't break everything).

@Jack-Works
Copy link
Contributor

@Jack-Works
Copy link
Contributor

Here is my auto generated type definitions. I will improve the generated files these days.

@Jack-Works
Copy link
Contributor

Ok. https://github.com/Jack-Works/matrix-js-sdk-type

image

Finally my work ends. But the type is very, very incomplete and I have no patience to continue working on the typescript declaration generation. Wish matrix to offer their own declaration some day!

@huan
Copy link
Contributor Author

huan commented Feb 27, 2020

Just sent the PR for typing for Matrix JS SDK (matrix-js-sdk) to DefinitelyTyped.

See: DefinitelyTyped/DefinitelyTyped#42673

@huan
Copy link
Contributor Author

huan commented Mar 2, 2020

The PR is merged to the DefinitelyTyped repository.

now we can use @types/matrix-js-sdk by npm install:

npm install @types/matrix-js-sdk -D

There are lots of parts will need to be improved, so please not be too surprised when you ran into any type of mismatch problems with this typing.

@andrevmatos
Copy link

@huan I can't get it to work with [email protected], because it includes a "typings": "./lib/index.d.ts" entry in package.json, but this file is a very naive stub file which just shadows the @types one and makes TSC throws all over the place. Any way to ignore it and use the @types one?

@huan
Copy link
Contributor Author

huan commented Mar 5, 2020

@andrevmatos Hi, I installed it and found that it only has an any typing for the whole module.

So I think maybe you can send a PR to matrix-js-sdk and to see if the author will accept removing the typing from it? However, that will break some existing projects, which will require them to install the @types/matrix-js-sdk, and with a very high probability run into some type mismatch problems, which is a very bad situation for them.

Another way to work around this, I think if I was you, I'd like to... run a postinstall script with a sed to remove the typings entry from package.json? It's really ugly but I think it is the fastest way to ... "fix" that.

@andrevmatos
Copy link

@huan I managed to get around the issue by adding "paths": { "matrix-js-sdk": ["node_modules/@types/matrix-js-sdk/index.d.ts"] } to my project's (and dependant's) tsconfig.json compilerOptions, which directed tsc to use the types directly from @types, even if typings is present in matrix-js-sdk's package.json.
I also took the freedom to make some extension to the typings, needed by our project, at DefinitelyTyped/DefinitelyTyped#42861
Let me know if any change is needed, please.
The correct fix would be to eventually pull in the type definitions directly into matrix-js-sdk's repo and replace the currently useless types (tsc throws with them here).
Thank you.

@Jack-Works
Copy link
Contributor

Finally my work ends. But the type is very, very incomplete and I have no patience to continue working on the typescript declaration generation. Wish matrix to offer their own declaration some day! (me on Jan 3 2020)

Note: I change my idea and I'm going to continue to maintain the generated type definition

Please check out https://github.com/Jack-Works/matrix-js-sdk-type

@berniezhao11
Copy link

Since we have a @types/matrix-js-sdk, can Matrix team either remove the typing inside the SDK, OR combine the typings from @types into the SDK?

@t3chguy
Copy link
Member

t3chguy commented Apr 30, 2020

Neither will happen, the long-term goal is to move the entire SDK over to TypeScript.

Current blocker: #1229

@berniezhao11
Copy link

@t3chguy understood! Thanks.

@berniezhao11
Copy link

@t3chguy is this completed in V8? I checked release notes but haven't found anything related to typing. Thanks.

@t3chguy
Copy link
Member

t3chguy commented Aug 8, 2020

No otherwise this issue would be closed + you can see most of the files are still .js...

It is blocked on #1229

@bergben
Copy link

bergben commented Feb 21, 2021

@huan I managed to get around the issue by adding "paths": { "matrix-js-sdk": ["node_modules/@types/matrix-js-sdk/index.d.ts"] } to my project's (and dependant's) tsconfig.json compilerOptions, which directed tsc to use the types directly from @types, even if typings is present in matrix-js-sdk's package.json.
I also took the freedom to make some extension to the typings, needed by our project, at DefinitelyTyped/DefinitelyTyped#42861
Let me know if any change is needed, please.
The correct fix would be to eventually pull in the type definitions directly into matrix-js-sdk's repo and replace the currently useless types (tsc throws with them here).
Thank you.

@andrevmatos is this still working for you?

I am getting the following error:

Error: ./node_modules/@types/matrix-js-sdk/index.d.ts
[ng] Module build failed (from ./node_modules/@ngtools/webpack/src/index.js):
[ng] Error: [..]/node_modules/@types/matrix-js-sdk/index.d.ts is missing from the TypeScript compilation. Please make sure it is in your tsconfig via the 'files' or 'include' property.
[ng] The missing file seems to be part of a third party library. TS files in published libraries are often a sign of a badly packaged library. Please open an issue in the library repository to alert its author and ask them to package the library using the Angular Package Format (https://goo.gl/jB3GVv).

Note that I am using this within Angular, although that shouldn't cause the issue.

I have also tried adding "node_modules/@types/matrix-js-sdk/index.d.ts" to the "include" array in tsconfig, but that does not seem to have any effect.

A workaround to use the DefinitelyTyped types for now would be really helpful

@andrevmatos
Copy link

@bergben our current workaround is to remove this package's borked definitions at prepare time so TS will pick up DefinitelyTyped's

@Jack-Works
Copy link
Contributor

in case @types/matrix-js-sdk doesn't work well, https://github.com/Jack-Works/matrix-js-sdk-type has updated the generated types matching f547fa7 (v9.7.0+)

@bergben
Copy link

bergben commented Feb 22, 2021

@andrevmatos @Jack-Works thank you both for the quick answer.

I'll go with @andrevmatos workaround for now - I prefer using a source from DefinitelyTyped if available, but thanks anyway @Jack-Works!

@mattboardman
Copy link

mattboardman commented Apr 5, 2021

Another option that worked for me with the @types/matrix-js-sdk package:

// tsconfig.json
{
  "compilerOptions": {
    "paths": {
	"matrix-js-sdk": [
	  "node_modules/@types/matrix-js-sdk/index.d.ts"
	],
    }
  }
  "exclude": ["node_modules/matrix-js-sdk/lib/*.d.ts"]
}

@ShadowJonathan
Copy link
Contributor

Any progress with this? #1229 has been closed for unrelated reasons.

Could anyone start Just Converting this library into typescript? #1546 (comment) seems to suggest this is definitely possible.

@turt2live
Copy link
Member

We're already most of the way through a typescript conversion - if there are gaps, please open PRs.

@ShadowJonathan
Copy link
Contributor

Ah apologies, i had a very old version of js-sdk cloned locally, i forgot to update my comment.

There are some issues remaining wrt TS conversion, but yes, the majority has been done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants