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

feat: Add typescript support #1954

Closed
wants to merge 10 commits into from
Closed

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Jun 23, 2023

Pull Request

Issue

Currently Parse JS SDK relies on external typings

Closes: #1950

Approach

Copies DefinitelyTyped types into this repo

Tasks

  • test types
  • Add changes to documentation (guides, repository pages, code comments)

@parse-github-assistant
Copy link

I will reformat the title to use the proper commit message syntax.

@parse-github-assistant parse-github-assistant bot changed the title feat: add typescript support feat: Add typescript support Jun 23, 2023
@parse-github-assistant
Copy link

Thanks for opening this pull request!

@dblythy
Copy link
Member Author

dblythy commented Jun 23, 2023

@sadortun could you have a look at the approach here?

.github/workflows/ci.yml Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
@mtrezza mtrezza mentioned this pull request Jun 23, 2023
3 tasks
@dblythy
Copy link
Member Author

dblythy commented Jun 27, 2023

At the moment the d.ts files are just copied. I wonder if the better approach would be to convert this whole repo to typescript, perhaps it would resolve #1362

@mtrezza
Copy link
Member

mtrezza commented Jun 27, 2023

I agree, but how can we do that gradually in smaller PRs, instead of in 1 large PR? Also, we'd need to ensure that the API docs are still correctly generated, currently using JSDoc.

I think if we could start a simple PR with just converting 1 method (or 1 class?) to TS, while demonstrating the API docs generation still works, we'd have really solved what has been holding us back from adding TS support and can close #1950. It will be easy to find others to convert the rest, following such an example.

@sadortun
Copy link
Contributor

gradually in smaller PRs

That's the way to go! The repo already have a great pipeline, adding TS and ensuring the pipeline and 3rd party tools still work would be a huge step forward.

Converting the whole repo will take time (maybe a year?), and it'll be easier to troubleshoot problems with multiple small PRs.

@mtrezza
Copy link
Member

mtrezza commented Jun 29, 2023

@dblythy Would you want to:

  • rewrite the PR and try to convert just 1 method of a class to TS
  • see if the JSDoc generation still works, or if we need a new tool (TSDoc?)
  • add a CI check check-typescript which should fail on type conflicts, so we know whether we can merge these tiny PRs in the future to convert to TS

And discard the 3rd party TS file - it's not attached to the methods anyway and partly outdated, but we can keep a reference in the GitHub issue, as it may help others as a guidelines when converting to TS in their PRs.

I've also added a bounty to #1950 to hopefully get this going.

@dblythy
Copy link
Member Author

dblythy commented Jun 30, 2023

I've updated it so the d.ts file is auto-generated

I think the problem is that as types are imported across the project by Flow, all files will need to be updated to typescript in one go:

Screenshot 2023-06-30 at 4 10 41 pm

Screenshot 2023-06-30 at 7 03 19 pm

I've updated a few classes but the problem is they keep importing types from a file that isn't TS. And then when I change that file to .ts, the issue deepens.

The approach of this PR has been to:

  • rename files to .ts
  • Fix any ts issues in the file
  • Auto-generate definition files

It seems to be working pretty well.

@mtrezza
Copy link
Member

mtrezza commented Jul 1, 2023

You wrote "the issue deepens" and then "It seems to be working pretty well"; so I don't understand whether there is currently an obstacle or whether you've found an approach that works?

rewrite the PR and try to convert just 1 method of a class to TS

This PR contains a lot of changes. I wonder were these changes necessary in order to convert 1 method and fix subsequent TS errors, or were these just unrelated, additional TS conversions that would not have been required?

see if the JSDoc generation still works, or if we need a new tool (TSDoc?)

Looking at this change:

class Subscription extends EventEmitter {
  /*
   * @param {string} id - subscription id
   * @param {string} query - query to subscribe to
   * @param {string} sessionToken - optional session token
   */
  id: string;
  query: ParseQuery;
  sessionToken: string;
  subscribePromise: resolvingPromise<any>
  unsubscribePromise: resolvingPromise<any>
  subscribed: boolean;
  emit: any;
  constructor(id: string, query: ParseQuery, sessionToken: string) {

It seems that we have two (conflicting) type definitions for the same var; in JSDoc and in TS. Do we need to keep the @param and @returns entries in JSDoc to generate the API docs, or could we remove them in lieu of the added TS types by switching to TSDoc?

@dblythy
Copy link
Member Author

dblythy commented Jul 1, 2023

It works well in the sense of generating correct types. But the types aren’t usable if the code imports types from a non-ts file (which causes the typescript error).

A fair few of the changes are auto-generated definition files. 90% of the changes are just file renames.

This PR bloated out as I’m conscious that if we offer supported, incomplete types, a lot of the intellesense + current typescript support will disappear.

Most of the changes are minor type corrections, as typescript detects issues as soon as a file is renamed.

I’d assume conflicting types in JSDocs / typescript means that the docs are wrong and need to be updated as well.

@mtrezza
Copy link
Member

mtrezza commented Jul 2, 2023

the types aren’t usable if the code imports types from a non-ts file (which causes the typescript error)

That's exactly what we want to solve with this PR. Is there a way to convert only one method of a class to TS while maybe adding some annotations (like lint annotations) that could silence these warnings so that a CI types check only checks what is already converted to TS.

This PR bloated out as I’m conscious that if we offer supported, incomplete types, a lot of the intellesense + current typescript support will disappear.

We'd have to naturally accept that during the transition. This PR is to verify the concept, without making a lot of conversions, because we want to explicitly know if it's possible to convert only a tiny part (like 1 method of a class) with a working CI type check and the API docs being generated. I would maybe reduce the type conversions in this PR (or open an additional new one as PoC and merge this one later?), because that will be the sort of PRs that contributors should be able to submit.

I’d assume conflicting types in JSDocs / typescript means that the docs are wrong and need to be updated as well.

Ideally we could which to TSDoc already if it uses TS where specified and fall back to taking types from JSDoc annotations where no TS types are specified. We'd need to test out if that works.

@dplewis
Copy link
Member

dplewis commented Jul 22, 2023

@dblythy @mtrezza I'd recommend using allowJS: true in your tsconfig.json. This will allow both js and ts files. Type checking should only be done on TS files. Once all files are statically typed properly (and flow is removed) then we can generate the .d.ts files in a separate PR to remove DefinitelyTyped.

@dplewis
Copy link
Member

dplewis commented Jul 27, 2023

@dblythy What is the status of this PR? Do you mind if I contribute to it?

@dblythy
Copy link
Member Author

dblythy commented Jul 28, 2023

This PR has gone stale as large wholesale changes are not preferred for this repo, although, I'm not sure how it will be possible to offer non-broken inbuilt types without large changes (see #1985)

@dplewis dplewis mentioned this pull request Aug 6, 2023
2 tasks
@dplewis
Copy link
Member

dplewis commented Aug 22, 2023

Closing via #1985

@dplewis dplewis closed this Aug 22, 2023
@dplewis dplewis deleted the add-types branch August 22, 2023 21:05
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.

Add TypeScript support
4 participants