To be clear, I'm not looking to a perfect solution that can replace the listener wonderfully. What I said is if you or others have a solution already laid on there, why we don't just pick it and use it to replace the problematic listener?
And I also think It's unrealistic to ban people to invent any wrappers if the API is indeed not convenient enough or just with too much details that could be simplified in different contexts (and I believe you need to send mails like this again and again to hack all the new wrappers if you really dislike the approach). Anyway, back to the case, I think it's good to stop using it. I just hope people could discuss the practices we'd already invented here, so we could know what we better to know while rewriting the code, instead of fail into the same traps that you and others and me (yes, I have a specific wrapper made from the past mistakes, too) had suffered from. 2015-07-25 23:24 GMT+08:00 Tim Guan-tin Chien <[email protected]>: > Thanks for you comment! > > Those are good points, but I specifically don't want to do things like > I-have-a-prefect-library-developed-in-secret-everyone-should-use-it. > This e-mail is more like a lesson learned retro and ask people to > interact with the API directly or invent their own wheels with these > experiences in mind. At least stop using SettingsListener.observe() > without at least aware it's problems. > > There are a few points worth mentioning from my impl though: > > 1. Setting values can be read through one async method and one sync method. > 2. The sync method will throw if the value is not there. > 3. The async method will return a promise that would return to an > object that keep the values, not the values themselves. This is done > so that the same promise could be returned and kept by the callers, > but the values can be updated by the observer callback. > > I generally prefer an organic process from which we find the most > useful solution from marketplace of ideas. As long as ideas considered > our past mistakes, I don't really care if Gaia as a whole have higher > entropy or not. > > > On Sat, Jul 25, 2015 at 6:18 PM, Greg Weng <[email protected]> wrote: >> I support to deprecate it because of the issues you mentioned. However, when >> people tried to wrap an API again and again, it obviously something failed >> to make people happy to use the API without any wrappers, although some >> debts are from the time we don't have any better way to manage async flow >> like Promise promised. >> >> So, IMO we may better to find an usable (hopefully compact and nice to use) >> alternative before we wipe out the code use the listener.Maybe Tim could >> elaborate how to use his solution to solve the common issues in Gaia, with a >> gist or readme under the app, and we could replace the listener with that >> stepwise. Or, if it's not general enough, we could evaluate different >> solutions with clear descriptions. At least, we should have some clear >> practices about how to replace it with the semantically equal snippets >> manually manipulating the API, if we don't expect an one-man army to submit >> a 3K+ lines patch across different components to do that. Without such best >> practices, people involve in may submit and land different patches with very >> different way from callback to Promise to handle the similar issues in >> different components, which would definitely add the entropy of our >> codebase, not eradicate that. >> >> 2015-07-25 17:32 GMT+08:00 Tim Guan-tin Chien <[email protected]>: >>> >>> Hi, >>> >>> SettingsListener was one of the oldest piece of Gaia, and it even >>> reaches into Gecko b2g/. However for years it's problem has not been >>> widely communicated. >>> >>> == Problems == >>> >>> 1) The "default value" on the 2nd parameter >>> >>> It asks for a "default value" on the 2nd parameter, and it will use it >>> when there is no mozSettings available and when mozSettings returns >>> undefined. Both behavior does not make sense nowadays because: >>> >>> 1.1) It makes no sense to return a fail safe value when mozSettings >>> breaks. We also don't expect Gaia apps to run on environments w/o >>> mozSettings. >>> 1.2) It creates two or more places in the code base in which people >>> would need update the "default value". The only place the default >>> value should live is the common-settings.json, which is the preload >>> values of mozSettings. We should use it and bump the db version in >>> Gecko to make the new value follow into the profile, instead of >>> duplicate that value in the code base. >>> >>> 2) Races, start-up races everywhere >>> >>> The callback will be invoked when the value is first returned (via a >>> get() call), and when the installed observer invokes due to the change >>> in value. Before the first value returns, the value remain unknown in >>> the JS world but users of SettingsListener.observe() usually assume >>> user will not reach their code before getting that value. That's a >>> false assumption. The code ended up behaves incorrectly with an >>> undefined value (or, with an default value which assumed safe but may >>> not be). >>> >>> 3) It hides lock management from the caller >>> >>> The current implementation hides the lock inside the library, and >>> reuse if in the same function loop, but in fact lock exists to >>> facilitate ... lock on the database changes. There will be some >>> follow-up on the problem with mozSettings itself but in the mean time >>> we should stop using an helper library that hide locks and introduce >>> footguns around possible setting change races. >>> >>> == Proposal == >>> >>> To my surprises, people have not moving away from it already on other >>> apps. Either all the uses have carefully workaround the problems >>> stated, or the bugs are still there. >>> >>> My proposal here is simply deprecate SettingsListener and have each of >>> the users interact with mozSettings directly, or have each of the apps >>> develop libraries they feel comfortable. >>> >>> Async flow in JS is way better than what we have at the beginning of >>> the project, thank to mostly the introduction of Promise. We have also >>> learned a lot through bugs and regressions. Given the fact DOMRequest >>> is already a then-able, it's shouldn't be hard to manage the code and >>> make it only do things (or response to things) only until the setting >>> value is available. >>> >>> Even in just one System app, there are already multiple library-level >>> implementations that attempt to wrap mozSettings. Without saying which >>> one is the best one, since the problem with SettingsListener should be >>> understood, I would assume these implementations should not be >>> suffering the problem that SettingsListener have. >>> >>> I personally did a implementation in Keyboard app too [1], but it >>> might not be general enough. >>> >>> If this is a generally agreed path to move forward, we should start >>> doing it and new patches should stop using SettingsListener. >>> >>> We would have to deal with the same thing in Gecko too. I don't know >>> who will be responsible of that though. >>> >>> Thoughts? >>> >>> >>> Tim >>> >>> PS A related topic would be a thread named "mozSettings API refinement >>> proposal" started by Alive (hat tip!). Proposal was generally >>> perceived well but left unimplemented beyond DOMRequest#then. We >>> should get better at tracking these things. >>> >>> [1] >>> https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/keyboard/settings.js >>> _______________________________________________ >>> dev-gaia mailing list >>> [email protected] >>> https://lists.mozilla.org/listinfo/dev-gaia >> >> >> >> >> -- >> Greg Weng >> >> http://about.me/snowmantw >> >> Understand y f = f [ y f ] ; lose last remaining non-major friend >> -- Anonymous > _______________________________________________ > dev-gaia mailing list > [email protected] > https://lists.mozilla.org/listinfo/dev-gaia _______________________________________________ dev-b2g mailing list [email protected] https://lists.mozilla.org/listinfo/dev-b2g
