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

Confusing updateModel name #302

Open
StevenLangbroek opened this issue Jan 4, 2016 · 4 comments
Open

Confusing updateModel name #302

StevenLangbroek opened this issue Jan 4, 2016 · 4 comments

Comments

@StevenLangbroek
Copy link

So, funny story. About a year ago I wrote a really complex form. I haven't touched the form or Stickit since then. A bug was being reported, and we were seeing some weird behavior with (a) model changes being triggered twice, and (b) what i thought was a custom "set this dom change to this model attribute in this way" function had unintended side-effects.

We loop through certain fields in a model, and for each one add a binding to our form, the hash looks like this:

bindings[escaped_key] =
  observe : "reporting_api_credentials.#{key}.value"
  key : key
  update: ($el) -> $el.val()
  updateModel: (val, event, options) -> @setCredentials("reporting_api_credentials", options.key, val)

The confusing bit is, we misused the updateModel hook to run custom setter logic, without returning false, causing the model update logic to run again (both in our own setCredentials method, and because of not returning false in stickit core).

I feel this should be solved in the following way, and would love your input before I open a PR this week:

  • rename updateModel to shouldModelUpdate
  • offer actual setVal hook to change how attributes are being set
@StevenLangbroek
Copy link
Author

ping @akre54. Are you guys still maintaining this?

@akre54
Copy link
Contributor

akre54 commented Jan 5, 2016

Sorry, away on vaca... Stickit is on low-maintenance mode. Any quick and easy changes we can make will be accepted, big changes should necessitate a fork.

I'd rather not introduce a backwards-incompatible change like this, this late in the game. Can we maybe make the docs clearer in these areas?

@StevenLangbroek
Copy link
Author

Ah alright, no problem. We could add shouldModelUpdate, keeping updateModel, and add the setVal hook (which isn't breaking) under a "minor" release, and clarify in docs. Would you like me to make a PR for this?

@akre54
Copy link
Contributor

akre54 commented Jan 5, 2016

Aliases are confusing, but maybe in this case deprecating the former name would be ok (so long as we keep both around).

A PR would be great.

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

No branches or pull requests

2 participants