Skip to content

Mark parameters in knockout test as optional - #34699

Merged
weswigham merged 1 commit into
masterfrom
fix-ko-test-param-optionality
Apr 12, 2019
Merged

Mark parameters in knockout test as optional#34699
weswigham merged 1 commit into
masterfrom
fix-ko-test-param-optionality

Conversation

@weswigham

@weswigham weswigham commented Apr 12, 2019

Copy link
Copy Markdown
Contributor

Found while working on microsoft/TypeScript#30853 - the cb as written required ComponentInfo to be present, but the signature it's being assigned to has it as optional. Prior to the linked PR, the union with KnockoutComponentTypes.EmptyConfig hid this bug, as pretty much anything is assignable to the empty config.

@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Awaiting reviewer feedback labels Apr 12, 2019
@typescript-bot

typescript-bot commented Apr 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

@weswigham Thank you for submitting this PR!

🔔 @borisyankov @Igorbek @moonpyk @EnableSoftware @BenjaminEckardt @ffMathy @ltlombardi @Retsam - 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.

@Retsam

Retsam commented Apr 12, 2019

Copy link
Copy Markdown
Contributor

LGTM. FYI the latest minor version of knockout packages their types, and it looks like they have the same issue over there. It'd be great if you'd also open the same change as PR against their repo.

@typescript-bot typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. Merge:Express and removed Awaiting reviewer feedback labels Apr 12, 2019
@typescript-bot

typescript-bot commented Apr 12, 2019

Copy link
Copy Markdown
Contributor

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!

@weswigham
weswigham merged commit a8ef43c into master Apr 12, 2019
@weswigham
weswigham deleted the fix-ko-test-param-optionality branch April 12, 2019 19:55
This was referenced Apr 12, 2019
Closed
Merged
@mbest

mbest commented Apr 15, 2019

Copy link
Copy Markdown

The problem is actually that the type definition for createViewModel is wrong. Knockout always passes both parameters to the function.

weswigham added a commit that referenced this pull request Apr 15, 2019
@weswigham
Knockout always passes these params
ef849bf 
Following discussion about #34699 on knockout/knockout#2475
@weswigham weswigham mentioned this pull request Apr 15, 2019
Merged
alesn pushed a commit to alesn/DefinitelyTyped that referenced this pull request Apr 23, 2019
@weswigham @alesn
Mark parameters in knockout test as optional (DefinitelyTyped#34699)
3171b3c
alesn pushed a commit to alesn/DefinitelyTyped that referenced this pull request Apr 23, 2019
@weswigham @alesn
Knockout always passes these params
c2b42b6 
Following discussion about DefinitelyTyped#34699 on knockout/knockout#2475
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@borisyankov borisyankov Awaiting requested review from borisyankov borisyankov is a code owner
@Igorbek Igorbek Awaiting requested review from Igorbek Igorbek is a code owner
1 more reviewer
@Retsam Retsam Retsam approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

4 participants

@weswigham @typescript-bot @Retsam @mbest