Skip to content

Compatibility fix for keyof T in ts 2.9#25271

Merged
rbuckton merged 1 commit intomasterfrom
keyofCompatibility
Apr 25, 2018
Merged

Compatibility fix for keyof T in ts 2.9#25271
rbuckton merged 1 commit intomasterfrom
keyofCompatibility

Conversation

@rbuckton
Copy link
Copy Markdown
Collaborator

@rbuckton rbuckton commented Apr 25, 2018

This is a backwards-compatibility fix for keyof in light of recent changes to the default behavior of keyof in 2.9. The goal of this change is to update existing definitions with a type compatible with both TS 2.9 and earlier versions of TS. The only change in this PR that could not be made backwards compatible is the definition of Datastore.KEY in "google-cloud__datastore" as TS 2.9 adds further restrictions when indexing using non-unique symbol types.

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

If changing an existing definition:

@rbuckton rbuckton mentioned this pull request Apr 25, 2018
Merged
2 tasks
@rbuckton rbuckton requested a review from weswigham April 25, 2018 00:53
@fluggo fluggo mentioned this pull request Apr 25, 2018
Merged
9 tasks
@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Apr 25, 2018 •
edited
Loading

@rbuckton Thank you for submitting this PR!

🔔 @asvetliakov @Ragg- @beaulac @antonvasin @Aaronphy @nicolas-schmitt @pjo256 @robessog @tbayne @cdeutsch @rosskevin @walkerburgin @vsiao @danilojrr @Batbold-Gansukh @octatone @chengsieuly @mretolaza @katbusch @vitosamson @LKay @aaronbeall @jrakotoharisoa @flaub @alelode @UJosue10 @dawnmist @Ogglas @eugrdn @brmenchl @GiedriusGrabauskas @Chnoch @tkqubo @thasner @kenzierocks @clayne11 @tansongyang @NicholasBoll @mDibyo @pdeva @Graphcool @voxmatt @alloy @npirotte @sergey-buturlakin @mrk21 @vasek17 @ngbrown @awendland @KostyaEsmukov @johnnyreilly @DovydasNavickas @tkrotoff @huy-nguyen @grmiade @DaIgeb @egorshulga @neuoy @rraina @iskandersierra @mrapogee @Pajn @carsonf @aikoven @bancek @alsiola @tehbi4 @huwmartin - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). The Travis CI build failed labels Apr 25, 2018
@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Apr 25, 2018 •
edited
Loading

@rbuckton The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

weswigham
weswigham approved these changes Apr 25, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'm actually surprised there are so few copies of the old style of diff/omit.

@lkgarrison
Copy link
Copy Markdown

lkgarrison commented Apr 25, 2018

Thanks for the PR, this addresses a bug with the Cloud Datastore types where Datastore.KEY is typed as "symbol" instead of "unique symbol". Thanks!

@typescript-bot typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. Other Approved This PR was reviewed and signed-off by a community member. labels Apr 25, 2018
@rbuckton rbuckton force-pushed the keyofCompatibility branch from 12a4876 to b73b442 Compare April 25, 2018 02:49
@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Apr 25, 2018

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

@rbuckton rbuckton merged commit efbdc03 into master Apr 25, 2018
@rbuckton rbuckton deleted the keyofCompatibility branch April 25, 2018 03:51
@lkgarrison
Copy link
Copy Markdown

lkgarrison commented Apr 25, 2018

I can't find much information about versioning for this repo. I was curious what version of the Google Cloud datastore types would incorporate this change. Versioning happens at the package level rather than the repository level in this case, right?

@antonvasin antonvasin mentioned this pull request Apr 25, 2018
Merged
9 tasks
@ghost
Copy link
Copy Markdown

ghost commented Apr 26, 2018

@lkgarrison The published version of a package is the major.minor from line 1 of its index.d.ts file, plus an autoincremented patch version.

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

Reviewers

@weswigham weswigham weswigham approved these changes

@alloy alloy Awaiting requested review from alloy alloy is a code owner

@johnnyreilly johnnyreilly Awaiting requested review from johnnyreilly

@octatone octatone Awaiting requested review from octatone

@DanielRosenwasser DanielRosenwasser Awaiting requested review from DanielRosenwasser

+2 more reviewers

@aaronbeall aaronbeall aaronbeall approved these changes

@mhegazy mhegazy mhegazy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Other Approved This PR was reviewed and signed-off by a community member. Owner Approved A listed owner of this package signed off on the pull request. Popular package This PR affects a popular package (as counted by NPM download counts).

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@rbuckton @typescript-bot @lkgarrison @weswigham @aaronbeall @mhegazy