Details
Check the stamping / DB clearing logic on both platforms by logging in and then logging out
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| native/data/sqlite-data-handler.js | ||
|---|---|---|
| 45–49 ↗ | (On Diff #39243) | Adding this extra logic for error-handling based on web |
| 152 ↗ | (On Diff #39243) | Adding this check based on the logic in web, so that we clear the DB if we can't fetch its stamped user ID |
| 171 ↗ | (On Diff #39243) | This was the original check we discussed adding |
| web/shared-worker/sqlite-data-handler.js | ||
| 24 ↗ | (On Diff #39243) | Most changes to this file are just variable renames |
| 34 ↗ | (On Diff #39243) | I caught a typo here (setting instead of getting) and generally updated the comments to reflect the new language |
It seems like we can extract the logic to lib by creating a function that accepts three functions: for fetching the stamped user ID, clearing the DB, and stamping the DB. What do you think about doing this?
That could be a good idea. There are two alternatives I can think of:
- Expose those three functions on a context that is defined in lib, but provided separately on web and native. Then we could have a shared hook in lib that handles this instead of a function that accepts three functions.
- Expose those three functions via registerConfig. Then we could have a function in lib that handles this, but doesn't take any parameters.
I'll probably need to create some sort of shared interface for ENG-7680 and ENG-7681 that allows for clearing database state. In ENG-7775 I describe it a little bit:
introduce a mechanism where platform-specific SQLiteDataHandlers can report the conclusion of database deletion via some sort of event emitter model. Then we'll be able to return a promise that initiates database deletion and resolves once it's concluded.
I'm still thinking through what format that interface will take. I could consider including this functionality in that shared interface when I get there.
Note that a downside of this proposed approach is that we'll likely need multiple await getCommSharedWorker() calls on web. But looking at that function, it doesn't seem expensive.
| web/shared-worker/sqlite-data-handler.js | ||
|---|---|---|
| 52–53 ↗ | (On Diff #39243) | This is different between web and native, but could still be handled in a cross-platform way. The difference is that on native, we re-throw the error on this line. We could handle this by wrapping the entire handleSensitiveData call on web with a catch that just ignores the error |
I'm still thinking through what format that interface will take. I could consider including this functionality in that shared interface when I get there.
Sounds great!
Regarding context vs registerConfig - I think we should usually prefer the config because it is simpler. The context approach is necessary when the shared logic is placed inside hooks because these can't be shared via the config. But if the final solution uses the context it is also ok - I think it depends on the decisions from the mentioned tasks.
Note that a downside of this proposed approach is that we'll likely need multiple await getCommSharedWorker() calls on web. But looking at that function, it doesn't seem expensive.
Wondering if we can cache this somehow... if it's cheap, we probably shouldn't do that, as caching could cause some problems.
Regarding context vs registerConfig - I think we should usually prefer the config because it is simpler. The context approach is necessary when the shared logic is placed inside hooks because these can't be shared via the config. But if the final solution uses the context it is also ok - I think it depends on the decisions from the mentioned tasks.
Makes sense!
Note that a downside of this proposed approach is that we'll likely need multiple await getCommSharedWorker() calls on web. But looking at that function, it doesn't seem expensive.
Wondering if we can cache this somehow... if it's cheap, we probably shouldn't do that, as caching could cause some problems.
It's already cached in getCommSharedWorker :)