Thanks for opening this pull request!

import cycles involving ParseObject; help appreciated.

You mean circular dependencies?

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

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.

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.

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.

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

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

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

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

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)

@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,
  };
});

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

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.

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

Can this PR be closed? @dplewis @swittk

@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