-
Notifications
You must be signed in to change notification settings - Fork 283
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
feat: botbuilder stdlib #3066
feat: botbuilder stdlib #3066
Conversation
cf27fab
to
97d784a
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.
Looks good! Two small things in the package.json for botbuilder-stdlib.
24ce01f
to
10fd5ee
Compare
Pull Request Test Coverage Report for Build 401371168
💛 - Coveralls |
de4bceb
to
0ea72e2
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.
Temporarily blocking PR while we get other feedback
39aa573
to
3b3f451
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.
At the moment I think the doc strings need to be updated with xrefs
otherwise the expectation is that the developer is already familiar with the contents of botbuilder-stdlib
. I worry a little that a dev might wander in here, use the provided functions and introduce a bug due to expectations on either Node or TypeScript behavior that aren't the exact same as in this library.
This isn't a requirement for this PR as this package is essentially internal, but something to be considered for the future.
What do you think?
That's totally fair - my plan is twofold. First, carefully review the use of the type assertions/tests (especially |
1fd6763
to
91b64dd
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.
One small suggestion. but otherwise LGTM
Revert this change once sandersn/downlevel-dts#48 lands upstream
Co-authored-by: Steven Gum <[email protected]>
Also emits custom errors for undefined, mis-typing, etc. for better runtime detection. This enables us to construct assertions that work for `Partial` types.
455bb53
to
7c7135c
Compare
This PR introduces a new package,
botbuilder-stdlib,
that exports some helpful shared utilities. The type assertion code will provide a robust way to construct type assertions that perform runtime type validation and compile-time type hinting. The result will be more type-safe code and a developer experience that better leverages IntelliSense.A note for code reviewers: the
botframework-schema
diff is quite large due toprettier
code formatting changes. I performed the code formatting step as a separate commit before adding the new type assertions. Hopefully, that makes this a bit easier to review.I'm open to feedback on the developer ergonomics for asserting types. Unfortunately, the compiler requires that type assertions functions do not reference type parameters without an explicit type declaration (i.e.
assert.arrayOf(assert.string)
is not possible withoutconst arrayOfStrings: Assertion<Array<string>> = assert.arrayOf(assert.string)
). This means that chaining and other helper functions are basically impossible to write. If you notice some verbose type declarations, that's why.