feat: add operator node, source node, and watcher node in core module #228
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| @@ -0,0 +1,41 @@ | |||
| import { Observable, Subscription, SubscriptionLike } from 'rxjs'; | |||
|
|
|||
| export interface INodePorts { | |||
There was a problem hiding this comment.
One of the best programmers I know scolded me big time for using I/Interface in identifiers 😄 we cant do it! https://stackoverflow.com/questions/68503504/what-is-the-naming-convention-for-interfaces-and-classes-in-typescript
| @@ -0,0 +1,41 @@ | |||
| import { Observable, Subscription, SubscriptionLike } from 'rxjs'; | |||
|
|
|||
| export interface INodePorts { | |||
There was a problem hiding this comment.
Where useful, please provide a little doc bloc with the purpose of interface/type
| nodeType: 'operator'; | ||
| } | ||
|
|
||
| export type WatcherEvents = 'completed' | 'errored'; |
There was a problem hiding this comment.
Should be singular: "WatcherEvent" ?
| import { tap } from 'rxjs/operators'; | ||
| import { firstValueFrom, ReplaySubject, Subscription, SubscriptionLike, Observer, Observable } from 'rxjs'; | ||
|
|
||
| export class WatcherResult implements IWatcherResult, Observer<any> { |
There was a problem hiding this comment.
This, WatcherNode etc can be upgraded to its own file/colocate with a "WatcherNode" file?
| getPort(portName: string): Observable<unknown>; | ||
| } | ||
|
|
||
| export interface ISourceNode { |
There was a problem hiding this comment.
As we discussed, "Node" name is kind of already taken by the Diagram data structure.
We do have the Source/Operator/Watcher prefix here which is good.
As I understand it, now links will generate a operator node so we can handle counts, etc. Then, is it really a "Node" we are refering to?
We could consider a new name. Alternatively accept different "Node" concepts lives in three domains; ReactFlow, Diagram, Execution.
There was a problem hiding this comment.
Your suggestion is fantastic!
I believe renaming it to element might be a better choice.
| import { Console } from './console'; | ||
| import { take } from 'rxjs/operators'; | ||
|
|
||
| describe('Console', () => { |
There was a problem hiding this comment.
- We could consider instances/variables as lowercase always
- Historically, I just gave the ComputerConfigs capitalization based on their "importance" / central nature. But not sure that is a clean idea
- Files should always match case of primary export.
WDYT? Lets get a convention here
There was a problem hiding this comment.
- I agree with that point!
- Except for React Components, the rest could follow the first advice.
- The JS file name is part of the module identifier, so I believe it should adhere to a uniform standard rather than changing according to the content inside the file.
Maybe we can refer to Google's JS guide, what do you think? https://google.github.io/styleguide/jsguide.html#file-name
| events = new ReplaySubject<WatcherEvents>(); | ||
| subscription = new Subscription(); | ||
|
|
||
| public complete() { |
There was a problem hiding this comment.
Prefer no public as it is default?
todo: