orbital-sync icon indicating copy to clipboard operation
orbital-sync copied to clipboard

#190 - work on factory pattern ready for v6 codebase

Open timtjtim opened this issue 1 year ago • 1 comments

Motivation: basically everything about the specifics of the config schema that might change with v6 is to be handled by a specific class.

Detail: Sync does a lot less now - much more in Client and ClientV5. Slightly cleaner class file paths, but not sure if it should be x/v5/index.ts or x/xv5.ts e.g. host/v5/index.js vs host/hostv5.ts Run out of time today for e2e, integration tests, but want to get eyes on the code first before I pour more time into it in case we want to make large changes here.

To Do:

  • gravity unit tests
  • unit test coverage of new code
  • don't like that I've split notify, but it used specifics of the host configuration 😞
  • arguably the client.create method should move to the factory now
  • file envvars are tied to the path of the config key, which means they've changed - I think there should be a better way to do that so this doesn't break existing installs - sort of makes keeping it v5 compatible pointless!
  • one of the unit tests fails when run with yarn run test:unit but not when run with NODE_OPTIONS=--experimental-vm-modules yarn jest --runInBand test/unit/
    FAIL test/unit/sync.test.ts
      ● Test suite failed to run
    
        Jest worker encountered 4 child process exceptions, exceeding retry limit
    
          at ChildProcessWorker.initialize (node_modules/jest-worker/build/workers/ChildProcessWorker.js:181:21)
    
  • as above - e2e, integration
  • ?

timtjtim avatar Aug 21 '24 22:08 timtjtim

Took an hour this morning to go over this. Looks great! But didn't have enough time to figure out the failing tests. If you can get it passing, I'll happily merge 🙂

mattwebbio avatar Feb 09 '25 02:02 mattwebbio