Skip to content

allow then, andThen, catch, andCatch to be undefined#128

Merged
ryanbrainard merged 2 commits intoheroku:masterfrom
nfcampos:undefined-then
Jun 3, 2016
Merged

allow then, andThen, catch, andCatch to be undefined#128
ryanbrainard merged 2 commits intoheroku:masterfrom
nfcampos:undefined-then

Conversation

@nfcampos
Copy link
Copy Markdown
Contributor

@nfcampos nfcampos commented Jun 1, 2016

  • extended typecheck function to support more than on allowed type
  • extended typecheckCheck test function to support more than one allowed type

Comment thread src/utils/checkTypes.js Outdated
Array.isArray(types)
? types.some(t => typeof obj === t)
: typeof obj === types,
`${name} must be a ${types}. Instead received a %s.`,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This message will be weird if ${types} is an array. Probably need to do some joining. Also, it might be easier to convert types to an Array before the invariant to avoid having two special cases in here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

actually I tested it and an array is coerced to a string correctly, eg.

arr = ['function', 'undefined']
str = `then must be a ${arr}`

str will be then must be a function, undefined which is pretty clear but when types is an array I can replace a with one of so that it becomes then must be one of function, undefined, should I do that?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yeah, it would probably read better that way. (Of course this really isn't super important since it is only something shown in the dev console...)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

@ryanbrainard
Copy link
Copy Markdown
Contributor

Looks like the test needs to be updated.

@nfcampos
Copy link
Copy Markdown
Contributor Author

nfcampos commented Jun 2, 2016

fixed now

@ryanbrainard
Copy link
Copy Markdown
Contributor

Thanks!

@ryanbrainard ryanbrainard merged commit 117a4a7 into heroku:master Jun 3, 2016
@nfcampos
Copy link
Copy Markdown
Contributor Author

nfcampos commented Jun 8, 2016

@ryanbrainard could you cut a release with this?

@ryanbrainard
Copy link
Copy Markdown
Contributor

@nfcampos oh, sure, sorry I didn't already

@ryanbrainard
Copy link
Copy Markdown
Contributor

@nfcampos
Copy link
Copy Markdown
Contributor Author

nfcampos commented Jun 8, 2016

@ryanbrainard Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants