Conversation
|
|
||
| interface DataWithName { name: string; } | ||
|
|
||
| function bodyHasName(body: unknown): body is DataWithName { |
There was a problem hiding this comment.
why so complex?
I think the following is simpler, does not use coercion nor immutable variables.
function getName(body: unknown, ctx: FunctionContext): string {
if (typeof ctx.request.query.name === 'string') {
return ctx.request.query.name;
}
if (typeof body === 'object' && body && typeof body['name'] === 'string') {
return body['name'];
}
return 'World';
}
export const handler: BinarisFunction = async (body, ctx): Promise<string> => {
const name = getName(body, ctx);
return `Hello ${name}!`;
};
There was a problem hiding this comment.
I have to think about this. I'm not sure I entirely agree. There is more to consider than just writing the most concise code.
There was a problem hiding this comment.
Note that typescript compiler does not accept it in strict mode (E: Element implicitly has an 'any' type because type '{}' has no index signature.)
The unknown type almost forces coercion - perhaps it is too strict and we should publish the interface with any.
There was a problem hiding this comment.
100% agree. I think the existing code (my original + vlads revisions) is the best solution while we have body as unknown. If we change that component, your solution makes a lot of sense.
typescript-function/tsconfig.json
Outdated
| "esnext.asynciterable" | ||
| ], | ||
| "target": "es2017", | ||
| "module": "commonjs", |
There was a problem hiding this comment.
are all those needed?
most of them are defaults anyhow, not?
There was a problem hiding this comment.
These are literally our own rules from nodeutils copy pasted.
There was a problem hiding this comment.
Consider starting from tsc --init since that's the equivalent of npm init, bn create, etc.
There might be various differences - e.g. targeting node8, not having to publish the module.
| extends: | ||
| - 'tslint:recommended' | ||
| jsRules: | ||
| rules: |
There was a problem hiding this comment.
again, very very verbose.
longer than the code itself
There was a problem hiding this comment.
as above,
These are literally our own rules from nodeutils copy pasted.
rylandg
left a comment
There was a problem hiding this comment.
@vogre I switched to the default tsconfig.json (I added a outDir, include and exclude). Do you think it's better than what we had before?
Also, did we decide Michael's code is better or are we keeping it the way it is?
It seemed like an example we really needed. I kept it super simple (100% port of our template NodeJS function) but we can add more complex TypeScript functions later.