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

REFACTOR: Mitigate cyclic dependency between Jsonable classes #1792

Conversation

catloversg
Copy link
Contributor

All Jsonable classes depend on constructorsForReviver in src\utils\JSONReviver.ts, and JSONReviver has a complicated dependency tree due to loadActionIdentifier in src\Bladeburner\utils\loadActionIdentifier.ts. This tree can create a cyclic dependency between Jsonable classes.

Jest test:

import { Player } from "@player";

describe("test", () => {
  test("test", () => {
    expect(Player.bitNodeN).toStrictEqual(1);
  });
});

MRE:

  • In src\Settings\Settings.ts, import Stock from src\StockMarket\Stock.ts and use it.
  • Run Jest test above.

Log:

TypeError: Cannot set properties of undefined (setting 'Stock')

      276 | }
      277 |
    > 278 | constructorsForReviver.Stock = Stock;
          |                             ^
      279 |

      at Object.<anonymous> (src/StockMarket/Stock.ts:278:29)
      at Object.require (src/Settings/Settings.ts:7:1)
      at Object.require (src/utils/StringHelperFunctions.ts:1:1)
      at Object.require (src/ui/React/AlertManager.tsx:6:1)
      at Object.require (src/ui/React/DialogBox.tsx:1:1)
      at Object.require (src/Netscript/ErrorMessages.ts:4:1)
      at Object.require (src/Netscript/TypeAssertion.ts:2:1)
      at Object.require (src/utils/EnumHelper.ts:5:1)
      at Object.require (src/Bladeburner/utils/loadActionIdentifier.ts:4:1)
      at Object.require (src/utils/JSONReviver.ts:4:1)
      at Object.require (src/Player.ts:3:1)
      at Object.require (test/jest/test.test.ts:1:1)

Problem:

  • Player depends on Reviver in src\utils\JSONReviver.ts.
  • JSONReviver.ts depends on loadActionIdentifier in src\Bladeburner\utils\loadActionIdentifier.ts.
  • loadActionIdentifier.ts has a complicated dependency tree. In this case, the tree contains Stock in src\StockMarket\Stock.ts.
  • Stock.ts needs to access constructorsForReviver in src\utils\JSONReviver.ts, but constructorsForReviver is undefined at this point.

This PR mitigates this problem:

  • Move cyrb53 out of src\utils\StringHelperFunctions.ts: Cut AlertManager -> StringHelperFunctions.
  • Move handleUnknownError and handleGetSaveDataInfoError out of src\Netscript\ErrorMessages.ts: Cut ErrorMessages -> DialogBox.
  • Move Reviver out of src\utils\JSONReviver.ts: Cut JSONReviver -> loadActionIdentifier.

Another mitigation is to move constructorsForReviver out of src\utils\JSONReviver.ts to cut "all Jsonable classes" -> JSONReviver. This mitigation will change all Jsonable classes, so I have not implemented it.

@d0sboots d0sboots merged commit 05da0ef into bitburner-official:dev Nov 23, 2024
5 checks passed
@catloversg catloversg deleted the pull-request/refactor/mitigate-cyclic-dependency-between-Jsonable-classes branch November 24, 2024 03:14
antoinedube pushed a commit to antoinedube/bitburner-source that referenced this pull request Dec 2, 2024
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.

2 participants