Add support for widget injection in connect command #238
Conversation
refactor script injection format. added tests
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| for (const [label, value] of Object.entries(labels)) { | ||
| const match = label.match(re)?.groups | ||
| if (match) { | ||
| set(sections, [match.id, camelCase(match.key)], value) |
There was a problem hiding this comment.
nit: I think it would be clearer if there was a function whose purpose is to convert labels to an array of Record<string, string>. this function's responsibility is more strictly defined IMO, with concepts like key case not included.
then have a separate function which maps the record objects to a ScriptInjection explicitly:
const stringRecordToScriptInjection = (o: Record<string, string>) => ({
src: o.src, // throw on missing
defer: o.defer && o.defer === 'true',
...
})
Bonus - you can use Zod to verify and convert the schema including coercing to boolean.
There was a problem hiding this comment.
Ideally, it'd be parseScriptInjection = recordParser(def)
// recordParser = (def) => (Record<string,string >)=> T
// where def can be something declarative as Zod)
But I think it's a bit pre-mature to extract it and Zod is a bit tricky since it translate 'false' to true which might be correct in js, but we are in the context of configuration.
| ) | ||
| return Object.values(scripts) | ||
| .map(parseScriptInjection) | ||
| .filter((x): x is ScriptInjection => !(x instanceof Error)) |
There was a problem hiding this comment.
probably worth logging the error
|
|
||
| export const scriptInjector = (id : string, script: ScriptInjection) => { | ||
| const injectScript = (model:ComposeModel, service:string) => addScript(model, service, { id, ...script }) | ||
| const injectAll = (_serviceName:string, _def: ComposeService) => true |
There was a problem hiding this comment.
Are we talking about injectAll? The args are there for inferring the signature type, they are prefixed with _ as they are not in use
| const injectScript = (model:ComposeModel, service:string) => addScript(model, service, { id, ...script }) | ||
| const injectAll = (_serviceName:string, _def: ComposeService) => true | ||
| return { | ||
| inject: (model: ComposeModel, serviceFilter = injectAll) => |
There was a problem hiding this comment.
can return a function instead of an object with a single key. or even simpler IMO - remove the nested func and have a single function with combined args.
There was a problem hiding this comment.
I created it initially as a single function, the reason for creating the nested function is for currying the service (id, params) from the injection context (model, filter), otherwise it doesn't provide much value compared to using addScript (which is used externally) directly.
The reason for returning an object and not a single function was for better communicating the action.
This is a
This PR update the format of script labels to
preevy.inject_script.(script_id).(script_prop)=value+ tests.It also adds support for using Preevy widget in connect command