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.

Sign up for GitHub

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

Jump to bottom

fix: Gracefully handle empty root node for nested component #660

Merged
merged 3 commits into from Jun 12, 2021
Merged

fix: Gracefully handle empty root node for nested component #660

merged 3 commits into from Jun 12, 2021

Conversation

LinusBorg
Copy link
Member

@LinusBorg LinusBorg commented Jun 11, 2021 •
edited

This PR covers a kind of edge case.

Conditions:

  1. The component to test is nested in an element (so the parent element is not the mount wrapper)
  2. the component doesn't render anything (so $el is a comment node)
  3. The component is extracted with wrapper.findComponent(Component)
  4. The test tries to use extractedWrapper.find() or ExtractedWrapper.findAll()

This will fail with an error being thrown:

TypeError: this.element.querySelector(All) is not a function

This PR handles this case in a graceful way.

@LinusBorg
fix: handle find(All) on components with comment root node in edge case
fb46569 
Squashed commits:
[00c8592] fix: apply same fix for findAll
[acbc848] fix: handle find() when root node is placeholder comment
@LinusBorg
Copy link
Member Author

LinusBorg commented Jun 11, 2021 •
edited

Huh, tests failed to run because of styles issues. Do I need to run prettier manually? The run from lint-staged ddidnt see a problem locally, weird.

Will see what's wrong later

lmiller1990
lmiller1990 approved these changes Jun 12, 2021
View reviewed changes
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.

LGTM!

Yep, I just run prettier manually.

@lmiller1990 lmiller1990 merged commit d64698c into master Jun 12, 2021
@lmiller1990 lmiller1990 deleted the linusborg/handle-comment-rootnode branch June 12, 2021 08:16
cexbrayat
cexbrayat reviewed Jun 12, 2021
View reviewed changes
src/vueWrapper.ts
: this.element.querySelectorAll(selector)
: this.element.querySelectorAll
? this.element.querySelectorAll(selector)
: ([] as unknown as NodeListOf<Element>)
Copy link
Member

Choose a reason for hiding this comment

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

nit: is this cast really necessary here? I would have thought that returning a simple array would be enough.

Copy link
Member

@cexbrayat cexbrayat Jun 12, 2021 •
edited

Choose a reason for hiding this comment

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

I gave it a quick try, and it looks like it's unnecessary. I'll make a PR to remove it. (update: see #662)

Copy link
Member

Choose a reason for hiding this comment

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

My bad: it was necessary!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, tsd doesn't like it :-P

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

@cexbrayat cexbrayat cexbrayat left review comments

@lmiller1990 lmiller1990 lmiller1990 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@LinusBorg @cexbrayat @lmiller1990