-
Notifications
You must be signed in to change notification settings - Fork 0
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
workflow triggers #4
base: main
Are you sure you want to change the base?
Conversation
@@ -47,7 +56,7 @@ export class Workflows { | |||
|
|||
const docs = new Map<string, Workflow>(); | |||
filenames.forEach((fn) => { | |||
const w = yaml.load(fs.readFileSync(fn, 'utf8')); | |||
const w = this.parse(yaml.load(fs.readFileSync(fn, 'utf8'))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
このタイミングで parse
(の中でやっている処理を) するのではなく,validate
後の方が良さそうに思いました.
(Workflow
型であることは type guard 後に決定するため)
return typeof obj === 'object' && obj.version && obj.name && Array.isArray(obj.steps); | ||
static parse(w: any): Workflow { | ||
if (w) { | ||
w.on = parseTrigger(w.on); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validate
を type guard 関数へ戻すことを前提に,60 行目付近で以下のように呼び出す想定です.
if (this.validate(w)) {
w.on = parseTrigger(w.on); // ← ここでは on がエラーにならない
docs.set(w.name, w);
} else {
(※ parse
関数は削除します)
return w; | ||
} | ||
|
||
static validate(obj: Workflow): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
こちらは type guard 関数に戻したいです.
static validate(obj: Workflow): boolean { | |
static validate(obj: any): obj is Workflow |
static validate(obj: Workflow): boolean { | ||
return ( | ||
typeof obj === 'object' && | ||
typeof obj.version === 'number' && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(元々 'string'
を想定していましたが,しばらくは 'number'
でも困らないのでここままで OK です)
- workflow_dispatch | ||
- join | ||
- text: | ||
match: hello |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
基本的に項目の重複はないと思いますので,リストよりはハッシュ構造の方が良さそうです.
(text
等は複数発生するかもしれませんが,それは text
フィールド以下の属性で表現するイメージです)
@@ -148,6 +192,7 @@ export class WorkflowContext { | |||
private valid = true; | |||
private stepIndex = 0; | |||
private data: WorkflowStepData = {}; | |||
private readonly firstStep = { id: 'on' } as WorkflowStep; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
firstStep
を内部生成するのではなく,トリガーで発生した情報を this.data
に保存するのみにした方が良さそうに思います.
(resetByEvent
側の調整も必要)
if (current.id) { | ||
this.data[current.id] = { | ||
responder: res.message.user, | ||
...res.json, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
res.json
の中身を制限なく展開してしまうことは避けたいです.response.note
の方のみを使うようにできないでしょうか?
const newContext = workflows.createWorkflowContextByEvent(type, res); | ||
if (newContext) { | ||
await newContext.triggerWorkflow(type); | ||
return newContext; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
find の中で create は避けたいため,ここだけ別関数に切り出せないでしょうか?findCurrentWorkflowContext
の戻り値に基づいて create を呼び出すようなイメージです.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
例えば startWorkflow
に移してしまうとか.find でみつからなければ新規にワークフローを開始するイメージです.
} | ||
switch (type) { | ||
case 'text': { | ||
const res = e as Response<TextMessage>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WorkflowEventWith
から Response<*>
へのダウンキャストは少し大きすぎる感じがしました.
バイトデータならばバリデーションを経て特定の型にキャストすることは妥当なのですが,WorkflowEventWith
を用意したということはアクセス出来るフィールドに制限をかけたいはずなので,WorkflowEventWith
の定義をもう少し厳密よりにしてキャストを無くせると良さそうです.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
現状の WorkflowEventWith
は実質任意のオブジェクトみたいな感じなので,こちらを先に調整した方が良いかも知れません.
設計方針として,初期は制限を強くし,必要に応じて制限を緩める,が良いかなという印象です.
NoteUpdated: 'note_updated', | ||
NoteDeleted: 'note_deleted', | ||
} as const; | ||
export type WorkflowEventType = (typeof WorkflowEvent)[keyof typeof WorkflowEvent]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export type WorkflowEventType = (typeof WorkflowEvent)[keyof typeof WorkflowEvent]; | |
export type WorkflowEventType = typeof WorkflowEvent[keyof typeof WorkflowEvent]; |
8e709b8
to
b98edc9
Compare
github actions の on のように、イベントに応じてワークフローを動かすトリガーを定義できるようにしています。