Skip to content

Standardize "invalid input" behavior #2134

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

Closed
gibson042 opened this issue Mar 9, 2015 · 5 comments
Closed

Standardize "invalid input" behavior #2134

gibson042 opened this issue Mar 9, 2015 · 5 comments

Comments

@gibson042
Copy link
Member

Many jQuery collection operations are only meaningful on elements or (somewhat less commonly) DOM nodes, but the collections can logically contain anything. Such operations should always assume valid input, potentially throwing exceptions when they encounter an invalid member—especially given the ease of manually cleaning like $collection.filter("*").

  • Single-member read (e.g., .attr( name ), .css( name ), .offset())
  • Whole-collection read (e.g., .hasClass( clazz ))
  • Whole-collection write (e.g., .attr( name, val ), .css( props ), .offset( n ), .addClass( clazz ))

Special consideration is warranted for single-member reads responding to empty collections. Current behavior usually returns undefined, which seems reasonable to standardize on (including changing those methods that return null, like .scrollTop() and .width()). There are cases where this collides with real output, but such collisions seem to be sane—$el.css( unknownProperty ), $el.prop( nonexistent ), $hasNoData.data(), etc.

Related:

@dmethvin
Copy link
Member

I'm all for this in theory, but have the queasy feeling that there are probably cases where existing code is depending on undocumented fail-silent or other quirky behavior.

@timmywil
Copy link
Member

Per today's meeting, we agreed that we'd like to try out some changes and see how it goes. Let's nail down all of the places we'd like to see this change.

@timmywil
Copy link
Member

I think we all agree here, but we'll need to address each change on a case-by-case basis and assess its impact on the community. Rather than making sweeping changes across the board and asking everyone to make lots of adjustments to their code, we're going to do this incrementally, in stages. The niche use cases can be done first with a few of the more common cases being introduced every major release. We already have some changes in the pipeline. For any other changes, let's have specific issues opened for each one. I'm closing this issue because we've reached a consensus and now it's too broad.

@gibson042
Copy link
Member Author

You closed this without making more specific tickets... are we just going to independently address unsolicited pull requests removing guards?

And even if the answer to the above is affirmative, I think we should include the principle—"assume function input matches a documented signature"—somewhere in the repository for future reference. Maybe CONTRIBUTING.md?

@timmywil
Copy link
Member

Right. What I'd like to move away from is leaving issues open that could not be closed by a single pull request. Opening an issue for discussion is fine, but once the discussion resolves, it should be closed. We can track the broader guidelines and overarching goals in the Roadmap.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

3 participants