Typescript

[swittk]

Pull Request

• Link this pull request to an issue.

Issue

Closes: https://github.com/parse-community/Parse-SDK-JS/issues/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

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

[parse-github-assistant[bot]]

Thanks for opening this pull request!

[mtrezza]

import cycles involving ParseObject; help appreciated.

You mean circular dependencies?

[mtrezza]

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

[swittk]

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]

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]

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]

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).

[swittk]

@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]

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]

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

[dplewis]

@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]

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]

@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]

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

[chaiyen123]

@flovilmart

[swittk]

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]

@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.