4851 motion modal

[southerneer]

for #4851 and #4850

[southerneer]

The full functionality of this won't be QA-able until I complete #4850, but otherwise this is ready for review.

Actually, I'll just continue with #4850 on this branch since there's a lot of potential conflict.

[southerneer]

This is mostly ready to go. @aksonov @bengtan please feel free to review. I plan to do some more testing tomorrow before submitting for final approval/merge.

A few notable changes:

• I got rid of the onboarded flag on ClientData. Since we need to check permissions in the onboarding flow on a per-device basis anyway, it makes sense to store that flag locally on the device.
• I replaced PermissionStore with an observable.map and a couple helper functions in utils/permissions.ts. It's about half the code and it avoids unnecessary churn on our MST stores.

[southerneer]

This init fires a rnbgl check location call which automatically checks for location and motion permissions. For fresh installs this is bad because it fires the motion permission before the user press the "Allow Fitness & Motion" button. This change (moving init inside the reaction) solves for that case, but please review for other potential pitfalls.

[southerneer]

In the course of working on this I ran into #4871 . I'll think through the UI implications and work with Alan on a solution tomorrow.

[bengtan]

First impressions: There's a lot of changes. There's a lot of use cases to test. Code review isn't going to catch all the corner cases. Hence, I wouldn't object to just sending it out to QA and see what happens.

I'm not sure how much you want to try to get this PR into this week's release. If you want to squeeze it in, I'm okay with it.

However, if you are open to making larger/broader changes, I have some more comments/ideas. But if you take on any of these, it may mean you have to postpone the PR for another day or two or three.

I replaced PermissionStore with an observable.map and ...

I'm neutral on this. No objections as long as it works.

I got rid of the onboarded flag on ClientData

I see you created useDeviceOnboarded.ts as a replacement. And it has it's own AsyncStorage so it isn't lost during an upgrade/codepush.

How about generalising things so it can also store other persistent-across-upgrades variables instead of just one flag?

For example, there is a recent ticket #4849 which could also benefit.

(Or rather ... if you decide to generalise, then do ticket #4849 first and then rebase this PR on that?)

This init fires a rnbgl check location call which automatically checks for location and motion permissions. For fresh installs this is bad because it fires the motion permission before the user press the "Allow Fitness & Motion" button. This change (moving init inside the reaction) solves for that case, but please review for other potential pitfalls.

In LocationStore.ts, init is closely tied to the alwaysOn flag. I always found it (both the init function and the alwaysOn flag) a bit odd.

IIRC, we used to have a requirement 'only upload location if location is set to Always allow'. I think this is the rationale for the ... && self.alwaysOn condition in the reaction? But I dunno if this requirement is still relevant. I'm willing to get rid of this requirement if it means we can simplify the code.

alwaysOn is just a permissions check. It actually calls into the permissions subsystem. It's not something intrinsically related to RNBGL.

I think ... your changes won't negatively affect the operation of RNBGL. 'Should be fine' :)

However, in the interests of tidying-up, how about removing init and the alwaysOn flag from LocationStore? If needed, it can be relocated to the permissions subsystem (if it doesn't duplicate something that's already there?)

I think removing init and alwaysOn should be safe for LocationStore itself.

However, there are other parts (mainly UI code) which reference alwaysOn. We'd have to change them to reference the relevant flag from the permissions subsystem.

In the course of working on this I ran into #4871 . I'll think through the UI implications and work with Alan on a solution tomorrow.

I'm not merging this until @aksonov has also had a review.

But it sounds like merging should be postponed anyway until you've had time to think about 4781.

[aksonov]

A lot of changes and I agree with @bengtan that it is impossible to review for safe merge. What about QA will test codepush deeply first? And why don't split it into several PRs?

A few notable changes:

I got rid of the onboarded flag on ClientData. Since we need to check permissions in the onboarding flow on a per-device basis anyway, it makes sense to store that flag locally on the device.

Hm.. As I remember QA requested not to raise onboarded flow for existing users after reinstall so we moved onboarding flag to ClientData.

I replaced PermissionStore with an observable.map and a couple helper functions in utils/permissions.ts. It's about half the code and it avoids unnecessary churn on our MST stores.

I don't like this idea. Even more, I'm strictly against that. Main principle of stores (= classes) is encapsulation of all logic inside class. We could add more advanced logic later into store, but not into observable.map.

[aksonov]

And useDeviceOnboarded approach looks different from our existing MST infrastructure, I don't think it is good to add additional 'source of truth'. Why don't use MST and existing persistence mechanism? If we approve that then we will increase our code complexity...

I want to say again, I would like to split this issue into many. For example I believe you have strong reason to create useDeviceOnboarded, probably because current MST doesn't flexible enough and cannot satisfy your needs, so we need to create appropriate ticket to make our infrastructure flexible enough first. Otherwise we could finish with many such hooks in addition to our old data stores...

[aksonov]

I would say again and again - code like permissionStore.allowsNotification is better than permissions.get('allowsNotification') for code readability. It is not about auto-complete that works for both cases, but about code readability and code quality. Why properties (setters/getters) were introduced at all in programming? Because it allows code extensibility and encapsulation - we could add logic for setter or getter easily, but not for get or set method. The same principle applies to MST stores comparing to pure javascript functions. You could easily add some init/de-init code into MST store using onAttach/onDetach (a.k.a class), but not into functions. You can easily add some code logic that could be run when some data is changed into MST (like we are doing within navStore) but not into javascript functions... It is called reactive programming and it plays very nicely with MST. So persistence is not only benefit of MST, there any many many other benefits that is why it was created.

[southerneer]

I can change the code to be more readable or consistent with the code style in the rest of our app. The code quality is the same...it's strongly typed and no more or less error-prone than MST code. One advantage is that we don't need to worry about the typescript typing changing or degrading (mysteriously) if we add to it. Wocky.ts is often frustrating to work on because of this quirk.

It is called reactive programming and it plays very nicely with MST.

Mobx/observables is the reason MST is "reactive". The rest of MST's benefits (easily serializable, traceable, immutable, etc) don't apply to the simple case of permissions. MST adds a lot of overhead and structure that, in this case, is unnecessary. All we need is a handful of observable booleans, a way to trigger checking them, and a way to store the onboarded flag for later.

[southerneer]

How about generalising things so it can also store other persistent-across-upgrades variables instead of just one flag?

Given @aksonov 's feedback I'll try to limit the scope of this particular set of changes rather than expanding it. I agree, though, with the idea of pursuing device persistence holistically and including the onboarded flag as part of that change.

[southerneer]

In LocationStore.ts, init is closely tied to the alwaysOn flag. I always found it (both the init function and the alwaysOn flag) a bit odd.

Yeah, I think LocationStore.ts could use some refactoring. Maybe we can start a discussion ticket for that and/or talk about on our next call.

[southerneer]

What about QA will test codepush deeply first?

These changes involve onboarding and permissions and are therefore not capable of being tested via codepush.

[southerneer]

And why don't split it into several PRs?

I considered this 3 days ago. But I guess I'll do it now.

[southerneer]

Main principle of stores (= classes) is encapsulation of all logic inside class. We could add more advanced logic later into store, but not into observable.map.

The logic is still encapsulated...just inside permissions.ts instead of a MST store. I can't think of an example of "advanced logic" that we can do with MST that we can't do with mobx. And I think we're overusing MST for simpler cases and it makes our code unnecessarily complex. Since PermissionStore doesn't depend on other pieces of the store it made sense to simplify. All we need is to expose a few observable booleans and persist one boolean across app reloads. And by removing it from the MST structure we don't have to worry if we change the shape of our MST store later.

[aksonov]

The logic is still encapsulated...just inside permissions.ts instead of a MST store

I mentioned yesterday atleast two files where this logic is placed (hook + permission.ts). Thank you for all your answers but I still didn't received strong reasoning to implement such changes. All I see is quite subjective wordings that "MST adds a lot of overhead, difficult to maintain, etc." I don't agree with it and yes, my opinion is subjective as well. When I introduced PermissionStore, you approved it (?) and I don't understand why we need spend time on removing it just because it doesn't look "good" for you... As I said yesterday, there is better looking mobx-keystone framework, maybe there are many other 'better' frameworks too, but so far I don't see objective reason to migrate.