Typescript #2044

swittk · 2023-10-30T06:17:06Z

Pull Request

Link this pull request to an issue.

Issue

Closes: #2012

Approach

Renamed and rewritten most of the source files to .ts, along with correcting some types that appear to have been out of date.
Minor refactoring to alleviate import cycles preventing normal behaviour (currently has major issue with import cycles involving ParseObject; help appreciated.
Exports from Typescript are now export default, so tests have been edited to reflect the behaviour, const module = require(...).default is now const module = require(...).

Tasks

Add tests
Fix require cycle involving ParseObject
Pass 100% tests
Pass Integration tests (Need to fix require cycle first)

Fixes parse-community#1660 (Caused by passing a Parse.File into the LocalDataStore, which is unexpected)

…ic localDataStore)

parse-github-assistant · 2023-10-30T06:17:09Z

Thanks for opening this pull request!

mtrezza · 2023-10-30T17:51:33Z

package.json

@@ -55,6 +55,7 @@
    "@semantic-release/github": "8.0.7",
    "@semantic-release/npm": "9.0.2",
    "@semantic-release/release-notes-generator": "10.0.3",
+    "@types/facebook-js-sdk": "^3.3.9",


What's the purpose of this dep?

There's the file FacebookUtils.ts where the FB global is used.

mtrezza · 2023-10-30T17:53:25Z

import cycles involving ParseObject; help appreciated.

You mean circular dependencies?

mtrezza · 2023-10-30T17:54:09Z

@parse-community/js-sdk Any help to review this is appreciated.

…before ParseRole

swittk · 2023-10-30T23:35:33Z

import cycles involving ParseObject; help appreciated.

You mean circular dependencies?

Yes, you'll find that currently the failing tests involve classes that extend ParseObject that failed to initialize with the error Class extends value #<Object> is not a constructor or null.

swittk · 2023-10-31T13:28:22Z

I've tried using Madge to visualize the circular dependencies and I think the main things that should be of concern are the files named Parse${name}, since they require class inheritance for definitions.

I'm not sure if this is the right move, but is it possible for us to move off the instanceof pattern at some places? After inspecting the whole codebase I think encode and decode are used by lots of parts of Parse, and it would be really hard to avoid circular deps if these functions require all classes to be fully defined in their definition for instanceof to work.

mtrezza · 2023-10-31T13:50:40Z

We've added madge to the Parse Server CI, I wasn't aware we don't even have that in the JS SDK. Would you mind opening an issue + PR and add madge like it's added in Parse Server (see ci.yaml, package.json scripts)? It's basically just a copy/paste. This is an issue apart from TS that should be addressed separately.

swittk · 2023-10-31T20:27:37Z

Sure, will open a PR for that.
Meanwhile I did a hypothesis-testing branch to prove that the current issue is due to dependency cycles.
https://github.com/swittk/Parse-SDK-JS/tree/uglyrequires
This branch passes all tests, but the import statements are moved at various places into the function bodies to make sure the definitions are not needed at first. It's rather ugly though, and contains lots of require(...).default || require(...) statements since many of the tests still expect and mock module.exports rather than the typescript-generated default export (This might be a problem too, and I'm not sure if maybe we should switch to the export = ... rather than the Typescript export default syntax for better backwards compatibility).

dplewis

@swittk Thank you for picking this up! I added a few comments and nits

For the issue you are having with the unit tests try adding the following to the 3 test files that are failing. I hope this helps.

jest.dontMock('../ParseObject');

dplewis · 2023-11-01T23:26:53Z

src/RESTController.ts

@@ -332,3 +348,4 @@ const RESTController = {
 };

 module.exports = RESTController;
+export default RESTController;


This red symbol means you are missing a new line at the end of the file

There are a lot of new lines missing and potential other lint issues. We should switch from eslint to tsc (npm run build:types) in a separate PR.

dplewis · 2023-11-01T23:28:34Z

src/ParseSchema.ts

    }
    if (type === 'Relation') {
+      // TODO: Why does options exist here?
      return this.addRelation(name, options.targetClass, options);


options here can be removed as it is unused

Thank you for the clarification 👍

dplewis · 2023-11-01T23:30:11Z

src/ParseQuery.ts

      return this;
    }
+    // TODO: Typescript.. How does this work? Does this even make sense?
    return (this._xhrRequest.onchange = () => this.cancel());


This should be written as the following as it should return a ParseQuery. cancel is supposed to be chained like the other functions.

this._xhrRequest.onchange = () => this.cancel(); return this;

Ahh OK. Thank you.

swittk · 2023-11-02T00:09:25Z

@swittk Thank you for picking this up! I added a few comments and nits

For the issue you are having with the unit tests try adding the following to the 3 test files that are failing. I hope this helps.
jest.dontMock('../ParseObject');

Thanks. That fixed two of the tests.
There's still decode-test.js though, where there are statements of ParseObject.fromJSON.mock.calls preventing it from running successfully without mocks.

(Sorry; I'm not that used to working with Jest.)

dplewis · 2023-11-02T00:14:34Z

I just checked your last commit you are still missing jest.dontMock('../ParseObject'); in decode-test. You can also verify this from the CI build fail.

For ParseObject.fromJSON.mock.calls, jest.spyOn(ParseObject, 'fromJSON').mockImplementation(() => {}); should work. There are other examples of spying in the test suite for reference

swittk · 2023-11-02T01:58:18Z

Jest tests appear to be fixed.
But seems like the Integration test still fails due to the dependency cycles though.

dplewis · 2023-11-02T06:36:32Z

@swittk I saw your require approach but we could refactor and combine some of the files that have circular dependencies and re-export them.

madge ./src --extensions js,ts --circular

✖ Found 20 circular dependencies!

1) ParseObject.ts > EventuallyQueue.ts
2) ParseObject.ts > EventuallyQueue.ts > ParseQuery.ts > OfflineQuery.ts > ObjectStateMutations.ts
3) ParseObject.ts > EventuallyQueue.ts > ParseQuery.ts > OfflineQuery.ts > ObjectStateMutations.ts > ParseOp.ts
4) ParseObject.ts > EventuallyQueue.ts > ParseQuery.ts > OfflineQuery.ts > ObjectStateMutations.ts > ParseOp.ts > ParseRelation.ts
5) ParseOp.ts > ParseRelation.ts
6) ParseQuery.ts > OfflineQuery.ts > ObjectStateMutations.ts > ParseOp.ts > ParseRelation.ts
7) ParseObject.ts > EventuallyQueue.ts > ParseQuery.ts > OfflineQuery.ts > ObjectStateMutations.ts > ParseOp.ts > arrayContainsObject.ts
8) ParseACL.ts > ParseRole.ts
9) ParseObject.ts > EventuallyQueue.ts > ParseQuery.ts > OfflineQuery.ts > ObjectStateMutations.ts > ParseOp.ts > decode.ts > ParseACL.ts > ParseRole.ts
10) ParseUser.ts > ParseObject.ts > EventuallyQueue.ts > ParseQuery.ts > OfflineQuery.ts > ObjectStateMutations.ts > ParseOp.ts > decode.ts > ParseACL.ts
11) ParseObject.ts > EventuallyQueue.ts > ParseQuery.ts > OfflineQuery.ts > ObjectStateMutations.ts > ParseOp.ts > decode.ts
12) ParseOp.ts > decode.ts
13) ParseObject.ts > EventuallyQueue.ts > ParseQuery.ts > OfflineQuery.ts > ObjectStateMutations.ts > ParseOp.ts > encode.ts
14) ParseOp.ts > encode.ts
15) ParseObject.ts > EventuallyQueue.ts > ParseQuery.ts > OfflineQuery.ts > ObjectStateMutations.ts > ParseOp.ts > unique.ts
16) ParseObject.ts > EventuallyQueue.ts > ParseQuery.ts > OfflineQuery.ts > equals.ts
17) ParseObject.ts > EventuallyQueue.ts > ParseQuery.ts
18) ParseObject.ts > canBeSerialized.ts
19) ParseObject.ts > unsavedChildren.ts
20) ParseUser.ts > ParseSession.ts

EventuallyQueue
Replace ParseQuery with RESTController and move to ParseObject
ParseObject
Add unsavedChildren and canBeSerialized
ParseUser
Add ParseACL, ParseRole and ParseSession
ParseQuery
Add OfflineQuery
ParseOp
Can be combined with decode and encode and added somewhere maybe ParseObject?

An alternative would be revert those files with circular dependencies back to JS and we can tackle these issue in a separate PR so we can get this PR merged. (~44/62) files is impressive.

@mtrezza Thoughts?

swittk · 2023-11-02T13:48:50Z

Just asking.. I'm trying the refactors, but I'm encountering errors in the tests, since they expect to mock ParseObject (e.g. unsavedChildren-test.js, but with unsavedChildren defined together in the same file as ParseObject, I don't think you can do mocks of ParseObject, since it's already been loaded in that file.

(I'm starting to get the sense that the very separated, single function/class definition in each file might've originally been due to reasons related to testing)

dplewis · 2023-11-02T20:16:19Z

@swittk You would have to something like this but for it to work you would need to get rid of the dependency cycles first like the previous unit test issue. If you give me access to your fork. I can clean this PR up for you.

jest.mock('../ParseObject', () => {
  const { unsavedChildren, canBeSerialized } = jest.requireActual('../ParseObject');
  const mockParseObject = jest.requireActual('./test_helpers/mockParseObject');
  return {
    __esModule: true,
    default: mockParseObject,
    canBeSerialized,
    unsavedChildren,
  };
});

swittk · 2023-11-04T02:39:25Z

Thanks. I've added you as a collaborator to my fork.

swittk · 2023-11-16T22:50:21Z

Would using registration of currently heavily reused classes (ParseQuery, ParseACL) into CoreManager be considered as an option? (or a similar object/registry that allows for dynamic registration of classes/objects).

Since most of the instanceof checks and class instantiation simply need the definitions to be found when calling them, I think simply registering and getting the classes via CoreManager.get..., CoreManager.set... (e.g. CoreManager.getParseQuery()) might provide a solution that fits with how other existing components are registered.

swittk · 2023-11-19T00:52:26Z

@dplewis I've made another branch with my proposed changes by using CoreManager as a central registry for classes that are reused. Most of the circular dependencies are fixed (and the rest are easily solvable using similar methodology), and it's enough that the tests and integrations now all pass.
https://github.com/swittk/Parse-SDK-JS/tree/coreinjects

Edit : I've since refactored more on the mentioned branch and only 1 circular dependency remains which is ParseOp > encode (which I wasn't sure if I should refactor out using the same method). The tests all pass and the integrations run fine. I've also made another branch, https://github.com/swittk/Parse-SDK-JS/tree/fixops, where all circular dependencies are fixed and all tests and integrations pass.

mtrezza · 2024-05-16T16:47:30Z

Can this PR be closed? @dplewis @swittk

dplewis · 2024-05-17T05:03:03Z

@mtrezza I would keep it open. If anybody wants to help with typescript support everything they need is in this PR. It would be hard to find if it was closed. I’m too lazy to bookmark it lol. @swittk put hours into this it’s only right that we close it when we have full TS support on version 6 of the SDK

swittk and others added 26 commits January 23, 2023 23:05

Fixes pinning of objects that are not ParseObjects into LocalDataStore

c33579c

Fixes parse-community#1660 (Caused by passing a Parse.File into the LocalDataStore, which is unexpected)

added failing test for nested Parse.File (need to shim a more realist…

f02589c

…ic localDataStore)

eslint unusedvars

94ca52d

Merge branch 'alpha' into alpha

8ad9d51

wrap with jest.fn

42d3944

Merge branch 'alpha' into alpha

f27d043

isEnabled checks

4ea18e6

removed error message

fce7f31

Merge branch 'parse-community:alpha' into alpha

934e5dc

changed according to suggestions

7003c94

Merge branch 'parse-community:alpha' into alpha

ec57dab

Merge branch 'alpha' into alpha

8211212

Merge branch 'alpha' into alpha

ee0d595

Merge branch 'alpha' into alpha

d50b896

merge

dc9e705

bunch of ts

352c0ac

more

1ab6249

moar types

7188bff

even more typings

1b3ba9d

more simple files

9839439

comments

b2c27dd

added ts files to checks and fixed fixable strict compiler warnings

ee8adf3

more, localdatastore

a0d2683

added facebook types

3ca292c

added facebook types

0334a07

ok but failing tests

d1b2dde

mtrezza reviewed Oct 30, 2023

View reviewed changes

swittk added 2 commits October 31, 2023 06:15

reverted changes to tests, try another approach

4604451

slightly better typings and test correction

606061f

swittk force-pushed the typescript branch from b17194c to 606061f Compare October 30, 2023 23:27

almost all passes now just need those ParseObject definitions loaded …

b7a6b27

…before ParseRole

dplewis suggested changes Nov 1, 2023

View reviewed changes

fixed more failing tests.. almost there.

4273ade

all tests passing now

5d55147

dplewis added 5 commits November 9, 2023 21:39

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

7edbce7

revert unit tests

7fef8b7

clean up

1fb6900

pin package

823935d

convert all files to ts

3b8e4dd

This comment was marked as spam.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typescript #2044

Typescript #2044

swittk commented Oct 30, 2023 •

edited

parse-github-assistant bot commented Oct 30, 2023

mtrezza Oct 30, 2023

swittk Oct 30, 2023

mtrezza commented Oct 30, 2023

mtrezza commented Oct 30, 2023

swittk commented Oct 30, 2023

swittk commented Oct 31, 2023

mtrezza commented Oct 31, 2023 •

edited

swittk commented Oct 31, 2023

dplewis left a comment •

edited

dplewis Nov 1, 2023

swittk Nov 2, 2023

dplewis Nov 2, 2023 •

edited

dplewis Nov 1, 2023

swittk Nov 2, 2023

dplewis Nov 1, 2023 •

edited

swittk Nov 2, 2023

swittk commented Nov 2, 2023

dplewis commented Nov 2, 2023 •

edited

swittk commented Nov 2, 2023

dplewis commented Nov 2, 2023 •

edited

swittk commented Nov 2, 2023

dplewis commented Nov 2, 2023 •

edited

swittk commented Nov 4, 2023

This comment was marked as spam.

swittk commented Nov 16, 2023 •

edited

swittk commented Nov 19, 2023 •

edited

mtrezza commented May 16, 2024

dplewis commented May 17, 2024 •

edited

Typescript #2044

Are you sure you want to change the base?

Typescript #2044

Conversation

swittk commented Oct 30, 2023 • edited

Pull Request

Issue

Approach

Tasks

parse-github-assistant bot commented Oct 30, 2023

Thanks for opening this pull request!

mtrezza Oct 30, 2023

Choose a reason for hiding this comment

swittk Oct 30, 2023

Choose a reason for hiding this comment

mtrezza commented Oct 30, 2023

mtrezza commented Oct 30, 2023

swittk commented Oct 30, 2023

swittk commented Oct 31, 2023

mtrezza commented Oct 31, 2023 • edited

swittk commented Oct 31, 2023

dplewis left a comment • edited

Choose a reason for hiding this comment

dplewis Nov 1, 2023

Choose a reason for hiding this comment

swittk Nov 2, 2023

Choose a reason for hiding this comment

dplewis Nov 2, 2023 • edited

Choose a reason for hiding this comment

dplewis Nov 1, 2023

Choose a reason for hiding this comment

swittk Nov 2, 2023

Choose a reason for hiding this comment

dplewis Nov 1, 2023 • edited

Choose a reason for hiding this comment

swittk Nov 2, 2023

Choose a reason for hiding this comment

swittk commented Nov 2, 2023

dplewis commented Nov 2, 2023 • edited

swittk commented Nov 2, 2023

dplewis commented Nov 2, 2023 • edited

swittk commented Nov 2, 2023

dplewis commented Nov 2, 2023 • edited

swittk commented Nov 4, 2023

This comment was marked as spam.

swittk commented Nov 16, 2023 • edited

swittk commented Nov 19, 2023 • edited

mtrezza commented May 16, 2024

dplewis commented May 17, 2024 • edited

swittk commented Oct 30, 2023 •

edited

mtrezza commented Oct 31, 2023 •

edited

dplewis left a comment •

edited

dplewis Nov 2, 2023 •

edited

dplewis Nov 1, 2023 •

edited

dplewis commented Nov 2, 2023 •

edited

dplewis commented Nov 2, 2023 •

edited

dplewis commented Nov 2, 2023 •

edited

swittk commented Nov 16, 2023 •

edited

swittk commented Nov 19, 2023 •

edited

dplewis commented May 17, 2024 •

edited