refactor: extract Function abstraction and split domain/api types#6196
Conversation
| // Today only the read side (`listFunctions` / `getFunction`) lives here, plus shared mappers | ||
| // (`EVENT_TYPE_MAPPINGS`, `eventTypeMapper`, `toDeployedNangoFunction`). The eventual roadmap | ||
| // is to consolidate write-side operations (deploy, deactivation, conflict detection) here too, | ||
| // so callers stop reaching into the underlying tables directly. |
There was a problem hiding this comment.
what about a functions package? 🙃
There was a problem hiding this comment.
Considered it but felt early / didn't really have enough of a justification yet.
| search: string | undefined; | ||
| limit: number; | ||
| offset: number; | ||
| }): Promise<{ rows: DeployedNangoFunction[]; total: number }> { |
There was a problem hiding this comment.
should return a Result if it can fail
| input?: string; | ||
| returns: string[]; | ||
| json_schema: JSONSchema7 | null; | ||
| /** Cron expression. */ |
There was a problem hiding this comment.
is it a cron or some every X mins kind of expression?
| case 'on-event': { | ||
| const apiEvent = row.event ? EVENT_TYPE_MAPPINGS[row.event as DBOnEventScript['event']] : undefined; | ||
| if (!apiEvent) { | ||
| return undefined; |
There was a problem hiding this comment.
that's silently dropping row if unknown on-event type. Is it on purpose?
There was a problem hiding this comment.
It was but I realized this may screw up pagination. Changed to failure (using Result)
| last_deployed: Date; | ||
| source: FunctionSource; | ||
| event: string | null; | ||
| } |
There was a problem hiding this comment.
I would put FunctionRow in repository. It is a db related construct. Mappers are what converts the row into the domain types
There was a problem hiding this comment.
True, good feedback
| @@ -1,49 +1,10 @@ | |||
| import db from '@nangohq/database'; | |||
There was a problem hiding this comment.
I have used the naming convention ./models/records.ts or ./models/tasks.ts in other part of the codebase for the logic interacting with the db. Repository isn't a bad name but we should stay consistent imho
| ]) | ||
| .first(); | ||
|
|
||
| return row ?? undefined; |
There was a problem hiding this comment.
?? undefined is redundant I think.
| } | ||
| throw new Error(`Unknown event type: ${eventType}`); | ||
| } | ||
| }; |
There was a problem hiding this comment.
do EVENT_TYPE_MAPPINGS and eventTypeMapper need to be exported?
| return EVENT_TYPE_MAPPINGS[event]; | ||
| }, | ||
| toDb: (eventType: OnEventType): DBOnEventScript['event'] => { | ||
| for (const [key, value] of Object.entries(EVENT_TYPE_MAPPINGS)) { |
There was a problem hiding this comment.
can we do a reverse map instead of a for loop?
) Been adding a bunch of these functions to deal with the abstract concept of a function (sync_configs and on-event scripts behind the scenes). This is a refactor to clean things up a bit. - Move read-side function logic (`listFunctions` / `getFunction`) into a dedicated `packages/shared/lib/services/functions/` folder split into `repository.ts`, `service.ts`, `mappers.ts`, `index.ts` — sets up a clear home for the eventual write-side consolidation. - Moved `NangoFunction` types from `packages/types/lib/functions/api.ts` into `domain.ts` - Renamed `NangoXxxFunctionDeployed` → `DeployedNangoXxxFunction` — adjective-first reads more naturally and groups deployed variants together. - De-duplicate the on-event enum mapping: EVENT_TYPE_MAPPINGS / eventTypeMapper
Been adding a bunch of these functions to deal with the abstract concept of a function (sync_configs and on-event scripts behind the scenes). This is a refactor to clean things up a bit.
listFunctions/getFunction) into a dedicatedpackages/shared/lib/services/functions/folder split intorepository.ts,service.ts,mappers.ts,index.ts— sets up a clear home for the eventual write-side consolidation.NangoFunctiontypes frompackages/types/lib/functions/api.tsintodomain.tsNangoXxxFunctionDeployed→DeployedNangoXxxFunction— adjective-first reads more naturally and groups deployed variants together.