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

make _validate respect silent option #3884

Closed

Conversation

craigmichaelmartin
Copy link
Contributor

This is useful both for isValid - where the validity of a model can easily be ascertained without sounding the invalid alarm - as well as for set where silence is respected even when validating.

@willmooney3
Copy link

👍

@jridgewell
Copy link
Collaborator

👍, someone else want to weigh in?

@akre54
Copy link
Collaborator

akre54 commented Dec 16, 2015

I'm 👍 anytime we can move towards fewer side effects, but aren't we trying to get rid of the silent option? Maybe this should take the inverse? i.e. {trigger: true}?

@craigmichaelmartin
Copy link
Contributor Author

I'm happy to implement trigger instead for _validate, but then we introduce a state where events are not triggered by default. Currently all events fire unless silenced, right? This would create the opposite state where now some events are only fired when triggered (and you have to know which are governed by trigger, and which by silent). EG, model set would accept options for trigger (respected in _validate for firing invalid) and a silent (respected for not firing change and sort). I really like the idea of replacing silent with trigger, but only if its codebase-wide. Let me know how you want me to proceed.

@akre54
Copy link
Collaborator

akre54 commented Dec 16, 2015

The only place I know of where we require trigger by default is in Router#navigate.

Currently all events fire unless silenced, right?

Most cases where we have a silent option are in methods like Collection#set and Model#set, which are mutative and meant to have side effects.

I mostly don't like that the name isValid() seems to imply that it returns a boolean and that calling it is side effect-free, when in fact it triggers an event on failure. We could split out the _validate method into _validate and _validateAndTrigger, say, but I'm 👎 on adding another silent flag.

@jridgewell
Copy link
Collaborator

The only place I know of where we require trigger by default is in Router#navigate.

That everyone hates... Trigger by default.

@akre54
Copy link
Collaborator

akre54 commented Dec 16, 2015

For sure. I think it's the wrong default in that instance.

In my idea scenario, set and friends would trigger an event, isValid() wouldn't.

@craigmichaelmartin
Copy link
Contributor Author

  1. I completely agree that isValid should be silent by default. This PR was intended to be non breaking. Are you ok with this breaking change? remove side effects from isValid #3896 is a backward incompatible implementation for this problem.
  2. Are you ok with model set and friends being able to trigger an event even with {silent: true})?

Both of these were addressed with options being considered in _validate.

@xerona
Copy link

xerona commented Dec 18, 2015

👍 lgtm

@jashkenas
Copy link
Owner

I think there's a reason why silent isn't being checked here — probably because it would muddle cases where the silent is being passed for something else.

For example — you want to set a model's attributes silently, but you do not want to set an invalid model.

Probably not worth the breaking change.

Edit: The better breaking change would be to kill off "silent" altogether.

@akre54
Copy link
Collaborator

akre54 commented Dec 20, 2015

A very good point

This is useful both for `isValid` - where the validity
of a model can easily be gleaned without sounding the
invalid alarm - as well as for `set` where silence is
respected even when validating.
@craigmichaelmartin
Copy link
Contributor Author

probably because it would muddle cases where the silent is being passed for something else.

Yeah, though I think silent is already muddled and this at least moves toward consistency - it's not intuitive as to which events are and aren't supposed to be silenced; in your example, why the change would be silent, but not the invalid.

That said, I'd be really happy to kill the silent option if that was not meant tongue in cheek.

Lastly, a specific use case that started this was to find a way for isValid to be called without side-effects. Maybe something like a purer isValid and a loud ensureValid?

@craigmichaelmartin
Copy link
Contributor Author

What is the status of this PR as a candidate for 1.4? I think it is the right step until (hopefully) removing the silent option altogether in 2.0 :fingers-crossed: 😄

@jgonggrijp
Copy link
Collaborator

Closing in favor of #3906.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants