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

fix(types): emitted can return undefined #1431

Merged
merged 1 commit into from
Feb 12, 2020

Conversation

lucaswerkmeister
Copy link
Contributor

@lucaswerkmeister lucaswerkmeister commented Feb 6, 2020

I’m not sure how strict this project is about undefined types in general, but I found it confusing that the documentation advised to use expect(…).toBeTruthy() on a function (emitted( 'foo' )) that was typed to always return an array (i. e., always truthy).

It might be better to only add the |undefined to the overload with an event name, not to the overload that returns all events at once… not sure.

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

I suppose in theory it’s a breaking change? People who previously assigned the result of .emitted() to a variable with an explicit type might now get a type error. The expected use, inline within expect(), should be fine, though.

Calls like .emitted().foo[0] or .emitted('foo')[0] may also need to be updated, depending on TypeScript config – in the tests, I’ve used the non-null assertion operator.

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

@lucaswerkmeister
Copy link
Contributor Author

After seeing the impact this has on .emitted().foo[0] and .emitted('foo')[0], I’m no longer convinced this is worth it… up to y’all, feel free to close if you want.

@lmiller1990
Copy link
Member

lmiller1990 commented Feb 10, 2020

Did not review the codebase yet, but I don't think emitted can be undefined - it's an empty object, {}, by default, and emitted().foo is an empty array [] by default, isn't it?

Either way, appreciate the effort to making a contribution. What do you think? I can look more into this if there is an undefined case (if it can be undefined, the types should reflect this).

@lucaswerkmeister
Copy link
Contributor Author

If emitted().foo was an empty array by default, then it would be a bug to use expect( … ).toBeFalsy() on it, as the documentation advises, because arrays are always truthy, even empty ones. But as far as I can tell, it starts out as undefined.

@lucaswerkmeister
Copy link
Contributor Author

(emitted() itself can never be undefined, that’s true, but I don’t think this pull request claims that either. emitted(eventName) can be undefined, though.)

@lmiller1990
Copy link
Member

Right, I get it now. I think this makes sense. Is this modified test code supposed to be there? I think just having the typings is probably fine. If you can remove that, I'd be happy to merge this.

@lucaswerkmeister
Copy link
Contributor Author

Without the changes to the test code, I got a CI failure because TypeScript complained about values being possibly undefined, I think.

@lmiller1990
Copy link
Member

Ok, I think this makes sense. Thanks!

@lmiller1990 lmiller1990 merged commit b41a09d into vuejs:dev Feb 12, 2020
@lucaswerkmeister lucaswerkmeister deleted the patch-1 branch February 12, 2020 13:16
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.

2 participants