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

feat: added optional instance setting to allow consumers to choose si… #80

Open
wants to merge 1 commit into
base: release/1.0.0
Choose a base branch
from

Conversation

eacaps
Copy link

@eacaps eacaps commented Aug 25, 2023

I'm not thrilled with how this turned out, and I think the typing of IOrbViewSettingsInit might be slightly off now, so please let me know if you'd like me to make adjustments

@eacaps eacaps requested review from cizl and tonilastre as code owners August 25, 2023 20:44
Copy link
Contributor

@tonilastre tonilastre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I've just added a few comments regarding the Typescript and lint. Let me know what you think.

Comment on lines +47 to +50
> & { render?: Partial<IRendererSettingsInit> } & {
instances: {
simulator?: ISimulator | (() => ISimulator);
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
> & { render?: Partial<IRendererSettingsInit> } & {
instances: {
simulator?: ISimulator | (() => ISimulator);
};
> & {
render?: Partial<IRendererSettingsInit>,
instances?: {
simulator?: ISimulator | (() => ISimulator);
};

I would say that you don't need to join them with & - just append to the render interface. Btw instances should be optional, if not users would need to always initiate an empty object even if they don't want to override the simulator instance.

Comment on lines +577 to +580
private _createSimulator(simulator: ISimulator | (() => ISimulator) | undefined): ISimulator {
if(typeof simulator === "function") return simulator();
return simulator || SimulatorFactory.getSimulator();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think lint is not working for you because this should be indented + if should be wrapped in brackets. Can you check if husky is initialized and npm run lint / npm run lint:fix works.

Also, there is a cool Typescript trick you can use here:

type ICallbackOrInstance<T> = T | (() => T);

export type IOrbViewSettingsInit<N extends INodeBase, E extends IEdgeBase> = Omit<
  ...
  instances?: {
    simulator?: ICallbackOrInstance<ISimulator>;
  };
}


private _createSimulator(simulator?: ICallbackOrInstance<ISimulator>): ISimulator {
   ...
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that definitely looks nicer. is there a cleaner way to do the type check instead of

 if(typeof simulator === "function")  return simulator();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is the most clean way to check for the function. You could have a util function somewhere. Just as I wrote this comment to give you a link to which place is the best to have a utility function in, I found a function:

https://github.com/memgraph/orb/blob/main/src/utils/type.utils.ts#L87

You already have isFunction type guard check so you can use it here :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants