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

Generate Typescript from JSDOCs #1390

Closed
wants to merge 22 commits into from

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Aug 8, 2021

New Pull Request Checklist

Issue Description

Currently working on automating the generation of Typescript Definitions from the JSDocs that are defined in this repo.

Related issue: #528

Approach

Refactor a lot of the current JSDocs so that it generates correct typings.

TODOs before merging

  • Add entry to changelog
  • Add changes to documentation (guides, repository pages, in-code descriptions)
  • Fix generics
  • Add Typescript tests to CI pipeline
  • Add automated generation into CI pipeline

Hi all,

Not sure if my formatting is perfect but closes parse-community#1176 (removes pre. SDK 2.0 success functions), and adds documentation for LiveQuery triggers.

Also, for me, when I google "Parse Javascript SDK", it takes me to v1.11.0. Is there any way we can add "This SDK is outdated" or some other warning to point users to the latest SDK?

Thank you!
@dblythy
Copy link
Member Author

dblythy commented Aug 8, 2021

I would like to know whether my approach for this PR is the right direction before I continue to work on generics.

The goal is to get the auto-generated types/index.ts file to match the manually generated DefinitelyTyped one.

@dblythy
Copy link
Member Author

dblythy commented Aug 8, 2021

I would also expect that if we move to this and shift the example repo to ts, there will be a number of expected issues arise. The main one i've worked through is arguments on functions that are optional, and weren't marker so though JSDocs.

@mtrezza
Copy link
Member

mtrezza commented Aug 8, 2021

I believe a thorough discussion regarding this issue happened in parse-community/parse-server#7334. I believe the path to solve this has been pretty much laid out for anyone who wants to give it a try.

@dblythy
Copy link
Member Author

dblythy commented Aug 8, 2021

I think the thing that is missing from this seems to be the fact that the types for Parse.Object, Parse.Cloud, need to come from the repo that require('parse') comes from. The parse server types come from require('parse-server'). Addressing the type issues in Parse Server is seperate from the JS SDK, imo.

Anyway, from your comments I see that you recommend running flow-to-ts first? This converts the whole src to .ts, and then creates definition files from that. Which would require a contributor to check and refactor the entire codebase, and then check and refactor all the definitions. That's a big job.

I think the first stage for TS support would be Typescript Definitions automated from the JSDocs (so that external devs can use this package without installing DefinitelyTyped, and any PR changes in here will automatically update types), and then later convert to ts. This way we can get better documentation coverage for things that TS shows that we've missed (i've added a bunch of methods here), and incrementally work towards typescript migration.

@mtrezza
Copy link
Member

mtrezza commented Aug 9, 2021

I think the first stage for TS support would be Typescript Definitions automated from the JSDocs

That was also my initial thought, but eventually I was convinced otherwise. The reason is that it seems to be challenging to reliably generate quality TS from JSDoc, at least with the framework that I tried.

The converstion from flow to ts, then generate JSDoc from TS seems to be much more reliable. So correcting all the flow defs (of which many already exist) would be the major part of the work. That turned out to be the least amount of work with the best results, in our discussion.

Whatever the approach is you want to follow, I recommend a proof of concept to estimate the scope of work. That made me abandon parse-community/parse-server#7335.

@dblythy
Copy link
Member Author

dblythy commented Aug 9, 2021

The reason is that it seems to be challenging to reliably generate quality TS from JSDoc

Just FYI the type defintions in this PR are automatically generated from the existing JSDocs. I just needed to do a fair bit of refactoring to JSDocs to make sure the right parameters were passed through, and that fields were marked as optional, etc. There were a few mistakes in JSDocs that this PR picked up, such as Parse.Object().createdAt having a property createdAt (e.g Parse.Object().createdAt.createdAt). To me it seems that reliable TS is generated IF the docs are accurate in their typedefs, interfaces, etc. Once I worked through and fixed all this up, the types were perfect. So in my opinion, this is a good approach is it ensures accuracy for the documentation, as well as referenced types

If you think I should abandon this approach then sure that's fine, however to me it seems that the generated types cover more of the SDK than the current Definitely Typed definitions, and seem to be pretty good quality.

This way too, when a new SDK feature is added, JSDocs will need to be added, as well as TS tests. If the TS tests fail, then it can assumed that the JSDocs configuration is invalid. This way we can essentially verify JSDocs and TS definitions in one go.

@mtrezza
Copy link
Member

mtrezza commented Aug 9, 2021

To me it seems that reliable TS is generated IF the docs are accurate in their typedefs, interfaces, etc.

That's good to hear, let's see how it goes! Any step towards TS is a good step, I'd say :-)

@dblythy
Copy link
Member Author

dblythy commented Aug 9, 2021

The main thing holding this PR back is support for generics and passing ts tests. If anyone has any skills in these areas let me know 😊

@dblythy
Copy link
Member Author

dblythy commented Aug 17, 2021

@sadortun I have noticed that you have experience with typescript, if you would be willing to give some pointers with generics that would be greatly appreciated.

@sadortun
Copy link
Contributor

sadortun commented Aug 17, 2021

@dblythy of course, I'll be glad to help!

It's getting a bit late, and I have a few long upcoming days, I would suggest you have a look at parse-community/parse-server#7337 I'll try to summarize later tomorow or Wednesday.

@dblythy
Copy link
Member Author

dblythy commented Aug 17, 2021

Thanks @sadortun! No rush at all.

The approach I've taken here is to tediously update the JSDocs contents to make sure it's accurate. For example, currently, most of the Parse Cloud docs are set to return a function, when it should be a callback of Parse.Cloud.TriggerRequest.

By updating the JSDocs to what they should return, and then running JSDocs to TS, I've auto-generated a pretty solid definitions file, and covers more of the SDK than the existing types.

The main problem however, is current TypeScript support from DefinitelyTyped has a lot of generics. Adding Generics to the JSDocs makes the docs somewhat unreadable.

I've solved this by adding a or tag depending on which segment the JSDocs is generating for. However, there are some generics which I can't get to perfectly match the existing DefinitelyTyped.

If you have a look at the tests files (note that this is from the existing DefintelyTyped repo), most of the test pass, however as mentioned, the generics are giving me troubles. Mostly conditional generics such as:

new Parse.Object<{ example: string }>("TestObject");

(I can't work out how to tell JSDocs that this should be new Parse.Object<{ example: string }>("TestObject", {example: 'hello');

The goal of this PR would to autogenerate types off JSDocs, and then run the TS tests to make sure the new feature is properly documented.

@sadortun
Copy link
Contributor

As we discussed on the other PR, this seems like a very hard approach to follow and later maintain. The most straightforward way to support TS is to :

  1. Add flow types to the current code
  2. use flow-to-ts to generate .d.ts files.

I'm not sure that having all types defined in JSDocs will be that useful of the actual function signature types differs.

Also if you refactor the functions signatures with proper types an IDE like JetBrains will let you automatically update the JSDocs.

One more detail il that adding actual signatures will help improve right away the server code readability, and avoid later bugs. It will also ensure that any updates to the code are reflected immediately into the .d.ts, and we'll be able to use that on internal methods too.


Also keep in mind the other later contributors, of every contributor have to struggle to understand the wierd JSDoc syntax, it will reduce engagement and less people will want/be able to contribute.

@sadortun
Copy link
Contributor

I'll try to complete a file with definitions later today to give you a more detailed proof of concept of parse-community/parse-server#7337

Also for the most part, you can simply copy the @types/Parse Interfaces and templates into Parse code and it should work.

Have a look here for an example

https://github.com/GoPlan-Finance/parse-server/blob/pr-migrations/src/SchemaMigrations/Migrations.js

@dblythy
Copy link
Member Author

dblythy commented Aug 17, 2021

Hmmm. I’m not sure I generally agree, as I argued with @mtrezza, I think that updating our docs to properly document the return and function types is something that needs to happen anyway. Working through this PR has made me realise how many of the docs are outdated or invalid.

The long term contribution to this feature would just require people to properly document their feature. If they haven’t documented it properly, the auto-generated types file won’t match their definitions, and the tests.ts CI will fail.

Again you can see in my auto-generated index.d.ts that there are literally hundreds of definitions that haven’t been generated by the manual DefinitelyTyped repo. We constantly get issues from TS issues with from the DefinitelyTyped repo, when the issue is documented in JSDocs. This would solve this problem; if something is documented, then it will have types.

If people want to add the generics to their new features that’s up to them using the funky syntax.

Are you suggesting adding the flow types to every single JS SDK file, and then running flow to ts? Does flow to ts support generics?

@sadortun
Copy link
Contributor

Ok, I see your point! I never really noticed if the docs were outdated.

If you are able to get the types to work from the docs it's really going to be great !

I would suggest you try to match a few of theses to make sure it's possible to cover the important ones

If those few functions can work, we're in business 🚀 I'll have some time to help you with this in ~10-15 days. And I'll be able to test this on a couple different projects.

@dblythy
Copy link
Member Author

dblythy commented Aug 17, 2021

Legend, feedback really appreciated.

Perhaps I can try to get the docs and generics to a point where they cover what you mention, and then you could try them out in a project? Then we can touch base and decide the best approach going forward, whether it's continuing this PR, or using flow to ts?

@sadortun
Copy link
Contributor

Oh yeah!

If you can get the ParseObjrct.set() and Query.equalsTo, I think we are in business with your approach!

Maybe also test to return Promise<T | undefined> for Query.first()


I really would see this as also an opportunity to add flow types in functions signatures at the same time :) At least for the simple stuff. Itll improve the codebase for our internal usage.

But we can also do that later too ....

@dblythy
Copy link
Member Author

dblythy commented Aug 17, 2021

Hmmmmm. Would you mind showing an example for how flow types work? I'm all for adding them in here (or perhaps in preference to this PR) if it's a more sustainable approach.

My thoughts are ideally the long term approach is converting this whole project to TS. But I don't see this happening for a while. We've just got to work out which approach is best in the short term.

Also - not sure if I mentioned - does the flow to ts approach solve the generics challenges?

@sadortun
Copy link
Contributor

So, from what I understand, flow is just a lightweight Typescript. Most of the flow types are exactly the same as TS.

From what I saw in my PR, you just need to add //@flow at the first line of each files to have your IDE recognize it

You can have a look here, everything is fully typed with flow.

https://github.com/GoPlan-Finance/parse-server/tree/pr-migrations/src/SchemaMigrations

@dblythy
Copy link
Member Author

dblythy commented Sep 28, 2021

Should note, that although this PR isn't perfect, it adds a lot of definitions that are missed by the current types, such as #1415

@sadortun
Copy link
Contributor

sadortun commented Sep 28, 2021

this PR isn't perfect

Hey! It's one giant step in the right direction! It'll be much easier to adjust types after that ! And also set a standard for what new PRs must include as of types!

@dblythy
Copy link
Member Author

dblythy commented Mar 19, 2022

Phew, this is turning into a huge PR.

I'm working on adding generic support. Unfortunately the tests from the DefinitelyTyped repo do not completely cover the existing types, so we will have to look at increasing typescript coverage.

The good news is that this PR contains some much welcomed love to the docs. The docs are generally documented with any, rather than more helpful information such as request.sessionToken etc.

https://github.com/parse-community/Parse-SDK-JS/pull/1390/files#diff-013fdd22dce75025754c37fa92433c19cabbdc4c4748178083b79185bc68c86c

This also adds a lot of support to functions that haven't been manually added to the DefinitelyTyped repo, such as getRoleProtectedFields, verifyPassword, getPushStatus, etc.

@mtrezza mtrezza mentioned this pull request Jun 20, 2023
3 tasks
@mtrezza
Copy link
Member

mtrezza commented Jun 20, 2023

@dblythy You've put a lot of work into this PR and it would be great if we could extract what is ready for merge and merge it.

I also suggest to split this off from the larger effort of TypeScript migration. If this PR is the answer to #1950 that would be great. If not, then I would add TypeScript support in a separate PR. What'd your opinion on this?

@dblythy
Copy link
Member Author

dblythy commented Jun 23, 2023

Getting this to work with generics is a little bit too hard for now

@dblythy dblythy closed this Jun 23, 2023
@kishanio
Copy link

why was this abandoned? Atleast have typescript for ParseServerOptions would be great.

@mtrezza
Copy link
Member

mtrezza commented Jan 19, 2024

@kishanio see #2012

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 this pull request may close these issues.

4 participants