-
Notifications
You must be signed in to change notification settings - Fork 9
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
Spacewalk: Add type checking with TypeScript #50
Conversation
8e79f55
to
f67b356
Compare
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.
Some comments/explanations to go along with my spacewalk wrap-up.
cc @mapbox/platform
}; | ||
|
||
module.exports = { and, equals, 'if': _if, not, or, notEquals }; |
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.
Notice 'if': _if
here to deal with if
being a reserved word.
const template = { | ||
AWSTemplateFormatVersion: '2010-09-09', | ||
/** |
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.
These @type {object}
are needed to allow for subsequent addition of more properties on each of these objects. Otherwise tsc
treats each one as an independent type that should not have keys added/removed.
- [ServiceRole][14] | ||
- [Parameters][15] | ||
- [Properties][16] | ||
- [LambdaOptions][1] |
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.
Unfortunately this markdown file generated from documentationjs
has become a lot less readable because documentationjs
does not currently handle constructor
-level parameter annotations.
If we want it to, we should submit a fix for documentationjs/documentation#1091
@@ -32,7 +34,12 @@ const Lambda = require('./lambda'); | |||
* module.exports = cf.merge(myTemplate, lambda); | |||
*/ | |||
class QueueLambda extends Lambda { | |||
constructor(options = {}) { | |||
/** | |||
* @param {QueueLambdaOptions & LambdaOptions} options configuration options for the scheduled Lambda |
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.
Example situation of creating a union type for inherited constructor options.
@@ -2,18 +2,20 @@ | |||
|
|||
const Lambda = require('./lambda'); | |||
|
|||
/** | |||
* @typedef { import('./lambda').LambdaOptions } LambdaOptions |
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.
Neat - we can import the base Lambda
class's typedef
for its constructor options
arg.
@@ -0,0 +1,49 @@ | |||
import build = require('./lib/build'); |
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.
This file defines the methods and properties of module-level export, ie, everything that will be available at cf.[NAME]
after doing const cf = require('@mapbox/cloudfriend')
.
I am not fully sure why these could not be inferred by tsc
. Perhaps there is a way to make that happen but I could not figure it out, so explicitly defining everything in this index.d.ts
is how I had to do it.
import build = require('./lib/build'); | ||
import validate = require('./lib/validate'); | ||
import merge = require('./lib/merge'); | ||
import shortcuts = require('./lib/shortcuts'); |
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.
It is nice that we can require
our JavaScript modules and TypeScript/IntelliSense will use all the JSDoc annotations defined in those files/modules!
This PR adds JavaScript type checking using TypeScript via JSDoc comments.
It is part of a Platform Spacewalk and is not necessarily meant to be merged.