feat: Support instance context [fixes DXJ-541] #392
Conversation
README.md
Outdated
|
|
||
| `registerService` Gives an ability to register service without schema. Passed `service` could be | ||
|
|
||
| - Plain object. In this case all function properties will be registered as Air service functions. |
There was a problem hiding this comment.
I would suggest for consistency always write AIR uppercased https://fluence.dev/docs/build/glossary#air
README.md
Outdated
| `registerService` Gives an ability to register service without schema. Passed `service` could be | ||
|
|
||
| - Plain object. In this case all function properties will be registered as Air service functions. | ||
| - Class instance. All class functions without inherited ones will be registered as Air service functions. |
There was a problem hiding this comment.
why not allow to call anything with this low level API including inherited class methods. I would also suggest to call them class methods instead of class functions
There was a problem hiding this comment.
Agree with renaming.
The decision to forbid inherited props made to make an attempt for avoiding accidental calls of Object methods or Array methods or other prototype chain methods. It could be surprising and forces you to control every method in prototype chain to have robust code(Very hard or impossible if you use external lib).
| // TODO: load worker in parallel with avm and marine, test that it works | ||
| const worker = await getWorker("@fluencelabs/marine-worker", CDNUrl); |
There was a problem hiding this comment.
curious why not trying to do that right now? what's stoping you?
There was a problem hiding this comment.
It all about the bug i discovered in threads.js package 2-3 months ago.
In short terms, bug arises when between worker init and it's actual usage passes too much time.
That's why i want to accomplish this important optimization in an another task later. Because it could involve editing @fluencelabs/threads.js package.
| const marineDeps = await parallelLoadStrategy("/"); | ||
| const marine = new MarineBackgroundRunner(...marineDeps); |
There was a problem hiding this comment.
parallelLoadStrategy sound a bit generic and I don't know if it is a strategy. You actually load stuff with this function. maybe loadMarineDependencies I don't know. Also these two steps seem to be always together. Do you expect to ever need to call this function without passing args to MarineBackgroundRunner? In any case I would create a helper function e.g. initMarine or something that does both of these steps and returns marine
There was a problem hiding this comment.
Well, it's more about inversion of control. In other words, i'd like to make MarineBackgroundRunner unaware of how these deps are loaded. Thats why i want to explicitly do these on a separate lines. For example in unit tests these deps could be mocked or substituted somehow.
I cannot come up with more strategies for this case. So yeah, looks like it's better to avoid strategies and just use a helper function as you suggested.
| } | ||
| } | ||
|
|
||
| describe("User API methods", () => { |
There was a problem hiding this comment.
What do you think, maybe we need a simple test here that checks that inherited methods work if you use high level api? Do you have this test?
There was a problem hiding this comment.
Or you want to not allow this for high level api as well?
There was a problem hiding this comment.
Nice catch. Actually yes, i want to allow calling inherited methods in high level api. Because if not, it would be surprising for user that he passes ts validation and fails at runtime because his methods are inherited.
| // eslint-disable-next-line @typescript-eslint/consistent-type-assertions | ||
| const prototype = Object.getPrototypeOf(service) as ServiceImpl; |
There was a problem hiding this comment.
maybe move this directly inside if statement because it seems to be needed only if isClassInstance
No description provided.