Skip to content
This repository has been archived by the owner on May 21, 2020. It is now read-only.

Add TypeScript function example #56

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rylandg
Copy link
Contributor

@rylandg rylandg commented Mar 19, 2019

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.

@rylandg rylandg requested a review from michaeladda March 19, 2019 16:13
Copy link
Contributor

@michaeladda michaeladda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!


interface DataWithName { name: string; }

function bodyHasName(body: unknown): body is DataWithName {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}!`;
};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"esnext.asynciterable"
],
"target": "es2017",
"module": "commonjs",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are all those needed?
most of them are defaults anyhow, not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are literally our own rules from nodeutils copy pasted.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, very very verbose.
longer than the code itself

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above,

These are literally our own rules from nodeutils copy pasted.

Copy link
Contributor Author

@rylandg rylandg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants