refactor: Add TypeScript support #1985

dblythy · 2023-07-28T02:56:22Z

Pull Request

Report security issues confidentially.
Any contribution is under this license.
Link this pull request to an issue.

Issue

Closes: #1950

Approach

Tasks

Add tests
Add changes to documentation (guides, repository pages, code comments)

parse-github-assistant · 2023-07-28T02:56:24Z

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

parse-github-assistant · 2023-07-28T02:56:27Z

Thanks for opening this pull request!

dblythy · 2023-07-28T03:00:42Z

This PR uses allowJS: true to auto-generate d.ts files, however, quite a few issues occur when building types:

And a few more when running test:types:

dblythy · 2023-07-28T03:04:48Z

@dplewis I have added you to my repo if you would like to contribute. I'm not sure how we can resolve the typescript issues without making large changes to the codebase. The issue seems to be that one file depends on invalid ts, and when that file is fixed, it depends on another file with invalid ts, and so forth. @mtrezza?

codecov · 2023-07-28T03:06:45Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01% ⚠️

Comparison is base (549b7b3) 99.88% compared to head (b3e8d85) 99.88%.

❗ Current head b3e8d85 differs from pull request most recent head bd69ab0. Consider uploading reports for the commit bd69ab0 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##            alpha    #1985      +/-   ##
==========================================
- Coverage   99.88%   99.88%   -0.01%     
==========================================
  Files          61       61              
  Lines        6143     6117      -26     
  Branches     1491     1491              
==========================================
- Hits         6136     6110      -26     
  Misses          7        7

Files Changed	Coverage Δ
src/EventuallyQueue.js	`100.00% <ø> (ø)`
src/LocalDatastore.js	`100.00% <ø> (ø)`
src/Storage.js	`100.00% <ø> (ø)`
src/CoreManager.js	`100.00% <100.00%> (ø)`
src/Parse.ts	`100.00% <100.00%> (ø)`
src/ParseSession.ts	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dplewis · 2023-07-28T17:14:09Z

without making large changes to the codebase

@dblythy I have no problem reviewing large changes. This PR is just syntactic sugar without any functionality.

mtrezza · 2023-07-28T21:02:36Z

@dplewis I think dblythy was referring to the fact that we want to add TypeScript support incrementally without requiring large parts of the codebase to be converted at once, for the reasons described in #1950.

dplewis · 2023-07-28T21:28:23Z

@dblythy If you ignore test:types what else would be needed to merge this PR as is? As long as the project transcompiles and the tests pass I would count this as Typescript support incrementally.

@mtrezza Thoughts?

mtrezza · 2023-07-28T21:32:34Z

Can we also have a TS test in the CI that ensures that the parts of the code that are already converted to TS are type-correct? Otherwise we'll have a hard time reviewing the incremental TS PRs.

dplewis · 2023-07-28T21:51:04Z

@dblythy @mtrezza For this to work you need to fix the test suite and treat it like the unit tests. Bypass the Parse module and test against the files themselves. Luckly we already have the test suite from definitely typed

import ParseObject from 'ParseObject';
class GameScore extends ParseObject {
  constructor(options?: any) {
      super('GameScore', options);
  }
}

instead of

import Parse from 'Parse';
class GameScore extends Parse.Object {
  constructor(options?: any) {
      super('GameScore', options);
  }
}

mtrezza · 2023-07-28T21:55:51Z

I was referring to the CI job that currently fails, the "test:types": "dtslint --expectOnly types", npm script. How can we get dtslint to pass?

dplewis · 2023-07-28T23:49:38Z

@dblythy I only changed Parse.ts and ParseSession.ts. Here are the steps if you want to convert other files.

Change file extension to .ts
Remove @flow from file if it exists
Add file to ./tsconfig.json
npm run build:types
Fix types
Add / enable tests in types/test.ts
npm run test:types
Compare types to @types/parse

dplewis · 2023-07-29T00:19:19Z

src/Parse.ts is broken so you can't test against it until the entire project is typed correctly

I stand corrected, we can test against Parse.ts but first we need to fix these. We should also consider moving to es6.

dplewis · 2023-07-30T04:48:15Z

@dblythy I used allowJs to generate all the typings available with a ts-no-check to remove warnings. Regenerating the typings would remove the no-check, I set it up to prevent that and allow for incremental regeneration by adding files to tsconfig.

index.d.ts shouldn’t be changed for now as every release will have some typing like DefinitelyTyped

dplewis · 2023-07-30T15:29:53Z

@mtrezza I think this is ready for review

mtrezza · 2023-07-30T21:49:23Z

Nice, the checks pass. @dblythy What do you think?

So these are the instructions for incrementally convert to TS? If so, I'll copy them to #1950, so we have that documented for contributors.

index.d.ts shouldn’t be changed for now as every release will have some typing like DefinitelyTyped

What does that mean? Is there anything new we need to consider when reviewing PRs? If so, could you amend the instructions with what not to do, so it also serves as guideline for reviewers?

dplewis · 2023-07-30T21:56:33Z

What does that mean?

To be honest I don't use typescript and never published a typescript package. I don't think users will have to install npm install @types/parse after this code gets release. This is my assumption.

Instructions have been updated.

mtrezza · 2023-07-30T22:02:28Z

That's good point. We know that @types/parse is currently more complete than our gradual conversion. For developers that probably means an immediate loss of types and IDE auto-complete, if they don't use or cannot use @types/parse anymore. What happens if a package installs the Parse JS SDK with our partial TS conversion and also has the @types/parse package installed? Will one overrule the other? Will there be conflicts? Does @types/parse have to be uninstalled? That's info we should include in the change log when merging this.

dplewis · 2023-07-30T22:19:16Z

We would remove the package from DefinitelyTyped to avoid confusion.

mtrezza · 2023-07-30T22:54:26Z

Would it make sense to accept TS conversion PRs but not publish the type files on NPM just yet, i.e. not include them in the published npm package, only keep them in the GitHub repo? We could wait until we have reached a good level of TS conversion so that developers won't loose the current level of type coverage so drastically.

dplewis · 2023-07-31T00:14:18Z

so that developers won't loose the current level of type coverage so drastically

Since index.d.ts is copied directly from DefinitelyTyped no coverage is loss. As far as publishing to npm, we should plan for the next major release of the SDK

mtrezza

Looks good; maybe someone else could take a look at this as well before we merge? @dblythy @sadortun @radandevist

mtrezza · 2023-08-09T12:17:30Z

I think we can leave this open for another 2 days; if there are no opposing reviews we'll go ahead and merge.

dblythy · 2023-08-10T11:06:52Z

I'm concerned that the allowJS will allow broken types that will throw issues for developers. An example is that the type mixed (which is compatible with flow) is used throughout.

Also as previously mentioned there are a lot of cases where the types are wrong (optionals are not specified, there's wrong types throughout, etc)

I've also enabled a few of the tests because I think we should at least have a few of them activated if we're going to merge.

Another thing that I've observed, even though the Session file has been renamed to .ts, issues still occur when build is ran, as typescript can't interpret the types from ParseObject.

add tests Update ParseSession.js Update ci.yml

dplewis · 2023-08-10T12:39:16Z

I'm concerned that the allowJS will allow broken types that will throw issues for developers.

We should change the title of the PR to Add initial TypeScript support. I purposefully removed types/index.d.ts from the package.json to prevent issues for developers until the we have full TypeScript support.

issues still occur when build is ran, as typescript can't interpret the types from ParseObject.

There are a few reasons why I specifically chose ParseSession to be the first file to convert to TypeScript. It has few functions and the limited dependencies it uses are the most important in the project. Its a good place to start since we are about to go down a typing rabbit hole.

dblythy · 2023-08-10T12:45:20Z

Sounds good - apologies, I'll revert my changes

This reverts commit b59b111.

dblythy · 2023-08-11T03:53:43Z

LGTM ✅

Thanks @dplewis

mtrezza · 2023-08-12T02:44:32Z

Great to see this is ready for merge; just one last thing: it's unclear what "initial TypeScript support" is supposed to mean. Instead of adding "initial", if we want to communicate anything specific, we can spell it out in the changelog. I can amend the changelog entry in the commit message, so we don't have to write an unreadably-long PR title. Could someone compose a short sentence what we mean by "initial"?

dplewis · 2023-08-12T03:37:22Z

if we want to communicate anything specific

These are internal changes with no effect on the developers yet. Nothing much to communicate unless to you want to show TS progress through changelogs. We can treat this the same as CI changes.

mtrezza · 2023-08-12T13:28:45Z

Then this should not be a feat but a refactor PR. I have changed it, and since there won't be any changelog entry I also removed the "initial", since it will only show in the commit history and one can only guess what that could mean anyway.

Please confirm that this is correct and I'll go ahead and merge.

dplewis · 2023-08-12T19:16:32Z

@mtrezza LGTM!

parseplatformorg · 2023-08-27T22:51:02Z

🎉 This change has been released in version 4.2.0-alpha.5

parseplatformorg · 2023-09-16T00:08:07Z

🎉 This change has been released in version 4.3.0-beta.1

parseplatformorg · 2023-09-26T01:08:21Z

🎉 This change has been released in version 4.3.0-alpha.1

parseplatformorg · 2023-11-16T14:45:08Z

🎉 This change has been released in version 4.3.0

feat: add typescript support

728fe64

parse-github-assistant bot changed the title ~~feat: add typescript support~~ feat: Add typescript support Jul 28, 2023

dblythy added 2 commits July 28, 2023 13:01

Merge remote-tracking branch 'upstream/alpha' into add-typescript

cf3e63e

Update package-lock.json

7f7d4fd

dblythy mentioned this pull request Jul 28, 2023

feat: Add typescript support #1954

Closed

2 tasks

dplewis added 2 commits July 28, 2023 18:39

convert ParseSession to TS

11e1c76

convert Parse module to TS

dd1b439

add no check

455a1b6

dplewis requested a review from mtrezza July 30, 2023 15:21

dplewis added 2 commits August 6, 2023 15:57

remove comment

60ae2a6

remove extra comments

41a41b4

mtrezza approved these changes Aug 6, 2023

View reviewed changes

mtrezza changed the title ~~feat: Add typescript support~~ feat: Add TypeScript support Aug 6, 2023

sadortun approved these changes Aug 10, 2023

View reviewed changes

merge

b59b111

add tests Update ParseSession.js Update ci.yml

dblythy force-pushed the add-typescript branch from e001e7c to b59b111 Compare August 10, 2023 11:13

Update tsconfig.json

ecc3086

dplewis requested a review from mtrezza August 10, 2023 23:15

dblythy changed the title ~~feat: Add TypeScript support~~ feat: Add initial TypeScript support Aug 11, 2023

Revert "merge"

bd69ab0

This reverts commit b59b111.

mtrezza changed the title ~~feat: Add initial TypeScript support~~ refactor: Add TypeScript support Aug 12, 2023

mtrezza approved these changes Aug 14, 2023

View reviewed changes

mtrezza merged commit 54efcf5 into parse-community:alpha Aug 14, 2023

parseplatformorg added the state:released-alpha Released as alpha version label Aug 27, 2023

parseplatformorg added the state:released-beta Released as beta version label Sep 16, 2023

parseplatformorg added the state:released Released as stable version label Nov 16, 2023

refactor: Add TypeScript support #1985

refactor: Add TypeScript support #1985

Conversation

dblythy commented Jul 28, 2023

Pull Request

Issue

Approach

Tasks

parse-github-assistant bot commented Jul 28, 2023

parse-github-assistant bot commented Jul 28, 2023

Thanks for opening this pull request!

dblythy commented Jul 28, 2023

dblythy commented Jul 28, 2023

codecov bot commented Jul 28, 2023 • edited Loading

Codecov Report

dplewis commented Jul 28, 2023 • edited Loading

mtrezza commented Jul 28, 2023 • edited Loading

dplewis commented Jul 28, 2023 • edited Loading

mtrezza commented Jul 28, 2023 • edited Loading

dplewis commented Jul 28, 2023

mtrezza commented Jul 28, 2023

dplewis commented Jul 28, 2023 • edited Loading

dplewis commented Jul 29, 2023 • edited Loading

dplewis commented Jul 30, 2023 • edited Loading

dplewis commented Jul 30, 2023 • edited Loading

mtrezza commented Jul 30, 2023 • edited Loading

dplewis commented Jul 30, 2023 • edited Loading

mtrezza commented Jul 30, 2023 • edited Loading

dplewis commented Jul 30, 2023

mtrezza commented Jul 30, 2023 • edited Loading

dplewis commented Jul 31, 2023

mtrezza left a comment

Choose a reason for hiding this comment

mtrezza commented Aug 9, 2023

dblythy commented Aug 10, 2023 • edited Loading

dplewis commented Aug 10, 2023 • edited Loading

dblythy commented Aug 10, 2023

dblythy commented Aug 11, 2023

mtrezza commented Aug 12, 2023 • edited Loading

dplewis commented Aug 12, 2023 • edited Loading

mtrezza commented Aug 12, 2023 • edited Loading

dplewis commented Aug 12, 2023 • edited Loading

parseplatformorg commented Aug 27, 2023

parseplatformorg commented Sep 16, 2023

parseplatformorg commented Sep 26, 2023

parseplatformorg commented Nov 16, 2023

codecov bot commented Jul 28, 2023 •

edited

Loading

dplewis commented Jul 28, 2023 •

edited

Loading

mtrezza commented Jul 28, 2023 •

edited

Loading

dplewis commented Jul 28, 2023 •

edited

Loading

mtrezza commented Jul 28, 2023 •

edited

Loading

dplewis commented Jul 28, 2023 •

edited

Loading

dplewis commented Jul 29, 2023 •

edited

Loading

dplewis commented Jul 30, 2023 •

edited

Loading

dplewis commented Jul 30, 2023 •

edited

Loading

mtrezza commented Jul 30, 2023 •

edited

Loading

dplewis commented Jul 30, 2023 •

edited

Loading

mtrezza commented Jul 30, 2023 •

edited

Loading

mtrezza commented Jul 30, 2023 •

edited

Loading

dblythy commented Aug 10, 2023 •

edited

Loading

dplewis commented Aug 10, 2023 •

edited

Loading

mtrezza commented Aug 12, 2023 •

edited

Loading

dplewis commented Aug 12, 2023 •

edited

Loading

mtrezza commented Aug 12, 2023 •

edited

Loading

dplewis commented Aug 12, 2023 •

edited

Loading