Skip to content
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

Finish TypeScript Migration (BREAKING CHANGE) #510

Closed
wants to merge 27 commits into from

Conversation

schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented Dec 28, 2018

This PR finishes the Typescript migration of the Firestore Server SDK. As part of this:

  • I changed the Validator class to use free-standing functions. This gets rid of the dynamic method creation, which doesn't really work with strong typing
  • I changed the client to use unknown instead of any for the user-facing types (only internally, not in the types we shipped)
  • I brought the client more in line with the Web SDK and only support the direction/query operator spelling that is outlined in the type definition (for example, "ASC" no longer works). This lets me use a common property validation function (and is the only breaking change)
  • I removed the dependency on is to be able to use TypeScript's typechecking.

This is the PR that I am planning on merging, and it's also the PR that I am would like to address all feedback in (to reduce merge conflicts). Since it is not very easy to review, I split this out into a series of PRs that are more tightly scoped:

The individual PRs are not meant to compile/pass tests. There should not be anything in this larger PR (which should pass CI, fingers crossed) that is not part of the other PRs.

This is fairly low priority, so please take your time to review.

Fixes: b/117464450 and b/119350730

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 28, 2018
@schmidt-sebastian schmidt-sebastian changed the title Mrschmidt cleanupall Cleanup: Finish TypeScript Migration Dec 28, 2018
@schmidt-sebastian schmidt-sebastian changed the title Cleanup: Finish TypeScript Migration Finish TypeScript Migration Dec 28, 2018
@schmidt-sebastian schmidt-sebastian changed the title Finish TypeScript Migration Finish TypeScript Migration (BREAKING CHANGE) Dec 28, 2018
@schmidt-sebastian
Copy link
Contributor Author

schmidt-sebastian commented Dec 28, 2018

@JustinBeckwith / Node team:

This PR fails CI with the following errors:

/Users/mrschmidt/GitHub/googleapis/nodejs-firestore/dev/src/serializer.ts Array type using 'T[]' is forbidden for non-simple types. Use 'Array<T>' instead. (array-type)

Note that gts check passes (gts --version reports 0.8.0, even though 0.9.0 is installed), but npm run gts check -- as run by the CI -- does not (npm rum gts --version reports 6.2.0). gts fix actually converts Array<unknown> to unknown[].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants