Skip to content

Conversation

angelogulina
Copy link
Contributor

Refers to this

This MR suggests to improve the guideline that get() should (and cannot) be used in combination with exists() by removing it from its return type.

The following changes only affect TS users as they will now see an error when using exists() on the returned get() result (it's assumed that when get() cannot find an element, it will just throw). They will need to update the code to use find() which is the official recommendation.

Copy link
Member

@cexbrayat cexbrayat 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, but I think the same should be done to getComponent no?

import { find } from './utils/find'
import { isFunctionalComponent } from './utils'

type NonexistentDomWrapper<T extends Element> = Omit<DOMWrapper<T>, 'exists'>
Copy link
Member

Choose a reason for hiding this comment

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

nit: the name NonexistentDomWrapper sounds a bit weird to me. To my french ear, it sounds like the DomWrapper does not exist 😅 I think we should name it differently, but I don't have a great suggestion...

Copy link
Member

Choose a reason for hiding this comment

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

I had the same thought... if anything this should ExistsDomWrapper or NotNullableDomWrapper.

Since this type is not used much, could we just inline it?

get<K extends keyof HTMLElementTagNameMap>(
  selector: K
):    ): Omit<<HTMLElementTagNameMap[K]>, 'exists'>

^ Probably wrong but that would be the idea.

Copy link
Member

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Cool! We need to do this for getComponent too.

import { find } from './utils/find'
import { isFunctionalComponent } from './utils'

type NonexistentDomWrapper<T extends Element> = Omit<DOMWrapper<T>, 'exists'>
Copy link
Member

Choose a reason for hiding this comment

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

I had the same thought... if anything this should ExistsDomWrapper or NotNullableDomWrapper.

Since this type is not used much, could we just inline it?

get<K extends keyof HTMLElementTagNameMap>(
  selector: K
):    ): Omit<<HTMLElementTagNameMap[K]>, 'exists'>

^ Probably wrong but that would be the idea.

@angelogulina angelogulina force-pushed the 176-remove-exists-from-get-retur-type branch from f7af4a3 to 351157c Compare August 31, 2020 08:03
@angelogulina
Copy link
Contributor Author

@lmiller1990 @cexbrayat thanks for the suggestions!

Making it inline removes the problem regarding the name, which is cool!

Copy link
Member

@cexbrayat cexbrayat left a comment

Choose a reason for hiding this comment

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

👌

@lmiller1990
Copy link
Member

Love it, great job.

@lmiller1990 lmiller1990 merged commit 309af60 into vuejs:master Aug 31, 2020
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.

3 participants