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

Find helper results null when query has empty result #15

Merged
merged 1 commit into from
Feb 26, 2017

Conversation

pixelhandler
Copy link
Collaborator

Originally the find helper returns an empty NodeList when the query has an empty result. Alternatively if the query has only a single result, instead of returning the NodeList with one HTMLElement the find helper returns the only result the HTMLElement.

The check for an empty result is find('.not-present').length === 0. Since the helper is acting intuitive like either document.querySelector or document.querySelectorAll the return value should be similar. No result from querySelector is null and an no results from querySelectorAll is an empty NodeList.

Checking for a null result when a query is made for one matching element makes sense. The find helper doesn't know that the result should be one or more based on the query. So returning an empty NodeList seems less than ideal.

Returning null, HTMLElement or NodeList[HTMLElement] is the gist of this PR.

Alternatively we could use two helpers, find(string) null | HTMLElement and findAll(string) NodeList[] (the node list may be empty when the query has no matching result.

Fixes #13

@cibernox
Copy link
Owner

I'm not agains the idea of find/findAll actually. It seems a cleaner solution than having a helper that returns a NodeList sometimes and single element if it happens to be only one item in the list.

Also thinking in Typescript, having two monomorphic function would lead to better types and IDE assistance.

@pixelhandler
Copy link
Collaborator Author

Yeah I like find/findAll too, should play nicer with TypeScript

@sarjanen
Copy link

sarjanen commented Feb 25, 2017

I like this, my only concern with findAll is if integration and acceptance test selectors should be the same? Know that this is a addon but i guess that the main goal is that this should be the implemantation of Grand Testing Unification?
The RFC can change and the find/findAll make sense like @cibernox says. Merge 😉

Edit, actually when findAll would be nice in acceptance test to.

@pixelhandler
Copy link
Collaborator Author

@cibernox @Padchi @rwjblue perhaps merging this is only an iteration maybe we need two helpers later.

@pixelhandler
Copy link
Collaborator Author

Since we're iterating on what makes for good native test helpers I propose we merge in this PR and release v0.2.1 - then try out find(selector: string): null | HTMLElement and findAll(selector: string): NodeList which work using document.querySelector and document.querySelectorAll. on a 0.3.0 tag.

@pixelhandler
Copy link
Collaborator Author

The use case for having a null response is that it's weird to expect result instanceof HTMLElement || result.length > 1 as an assertion for presence of a result (not empty).

@pixelhandler pixelhandler merged commit 8633656 into cibernox:master Feb 26, 2017
@pixelhandler pixelhandler deleted the find-no-result branch February 26, 2017 06:54
This was referenced Feb 26, 2017
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