Tiered DI #52
Tiered DI #52
Conversation
# Conflicts: # packages/module/test/state/MockAsyncMerkleStore.ts # packages/sdk/test/appChain/AppChain.test.ts
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| public create(childContainerProvider: ChildContainerProvider): void { | ||
| noop(); |
There was a problem hiding this comment.
Disabling @typescript-eslint/no-empty-function for this line would be nicer than noop()
There was a problem hiding this comment.
Ok but then why do we have this rule when we don't want the thing that the rule forces us to do?
| @@ -40,7 +48,9 @@ const errors = { | |||
| export const ModuleContainerErrors = errors; | |||
|
|
|||
| // determines that a module should be configurable by default | |||
| export type BaseModuleType = TypedClass<Configurable<unknown>>; | |||
| export type BaseModuleType = TypedClass< | |||
| ChildContainerStartable & Configurable<unknown> | |||
There was a problem hiding this comment.
Would be nice if we did a merged interface for this joint type and used it in ConfigurableModule implements ... as well
| @@ -148,6 +161,16 @@ export class ModuleContainer< | |||
| } | |||
| } | |||
|
|
|||
| public assertContainerInitialized( | |||
There was a problem hiding this comment.
We should probably start avoiding in-class assertion methods since we've learned recently they dont work outside of classes/instances themselves. But out of scope here.
| this.protocol.registerValue({ | ||
| Runtime: this.runtime, | ||
| }); | ||
| // These three statements are crucial for dependencies inside any of these |
There was a problem hiding this comment.
can this be a multiline comment instead?
There was a problem hiding this comment.
no there's a eslint rule against that
| @@ -31,7 +35,9 @@ export class LocalTaskQueue implements TaskQueue { | |||
| [key: string]: QueueListener[]; | |||
| } = {}; | |||
|
|
|||
| public constructor(private readonly simulatedDuration?: number) {} | |||
| public constructor() { | |||
There was a problem hiding this comment.
probably can be removed all together
| @@ -253,7 +261,7 @@ describe("block production", () => { | |||
|
|
|||
| const numberTxs = 3; | |||
|
|
|||
| it("should produce block with multiple transaction", async () => { | |||
| it.only("should produce block with multiple transaction", async () => { | |||
There was a problem hiding this comment.
explained that in a previous PR, we can't execute multiple tests at once rn.
Maybe that has changed now though?
No description provided.