search

sproutcore / sproutcore

JavaScript Application Framework - JS library only
sproutcore.com
Other
2.15k stars 290 forks source link

Dev warning about invalid view state for _doShow() #1325

Closed jameschao closed 9 years ago

jameschao commented 9 years ago

Currently, if a view is in the ATTACHED_SHOWN state, it'll fire off a warning if _doShow() is getting called again (by _isVisibleDidChange).

However, if isVisible is a computed property, it's possible that it could change from true to true. This seems like a valid case. However, _isVisibleDidChange could call _doShow twice, causing a dev warning the second time as the view is already in ATTACHED_SHOWN state.

There are two possible fixes:

The first approach seems the more correct solution to me

jameschao commented 9 years ago

What do folks think about this issue? I can implement the fix but wanted to get feedback from others. Thanks.

publickeating commented 9 years ago

Hi James,

In general, I think I was a bit overzealous in adding all of those warnings, because they are private methods and not actually directly callable by the developer, but the purpose was to try and expose situations where code was running that didn’t need to be.

I think there are two general approaches that the framework can take:

1) each method is self protected, so that it may be called in any state with very flexible arguments. This approach would mean _doShow should have no warnings, expecting to be called at any time.

2) each method is unprotected and assumes it is being used properly with strict arguments. This approach would mean _doShow would expect to only be called to actually “do show”.

I tend to lean towards option 2), because it’s less code, it’s faster and it’s actually much easier to debug/less prone to error (I also am a strong believer that it aids in usability and memorability of the API). So with that in mind, I agree more with _isVisibleDidChange should check the view state before calling _doShow. It’s an extra conditional check in _isVisibleDidChange, but I’m sure that’s less effort for the interpreter than preparing another function scope which ultimately does nothing.

Thanks for bringing this up.

Tyler Keating tyler@sproutcore.com

On Feb 4, 2015, at 2:05 PM, James Chao notifications@github.com wrote:

What do folks think about this issue? I can implement the fix but wanted to get feedback from others. Thanks.

— Reply to this email directly or view it on GitHub https://github.com/sproutcore/sproutcore/issues/1325#issuecomment-72928236.

jameschao commented 9 years ago

Hi @publickeating, thanks for articulating this so well. I also do like the second approach that you mentioned, and will implement the change in _isVisibleDidChange like you said.