Conversation
This is used to communicate about what the LocalStorageQueue will look like, using mostly the types But because they are quite similar for InMemoryQueue we also add the unit test that should pass and some more precisions
camillecroci
force-pushed
the
cc/local-storage-queue
branch
from
08f31df to
1eb783c
Compare
rowanmanning
left a comment
rowanmanning
left a comment
There was a problem hiding this comment.
A few small thoughts but otherwise think this makes sense 👍 another thought:
Do we need a place to easily test client changes like this? Maybe as part of the module a simple HTML page we can run locally so we can actually try out local storage stuff in-browser
| @@ -0,0 +1,29 @@ | |||
| const { LocalStorageQueue } = require('../../../../lib/queue/local-storage-queue'); | |||
|
|
|||
| describe('InMemoryQueue (extends Queue)', () => { | |||
There was a problem hiding this comment.
nitpick: test name needs to change
There was a problem hiding this comment.
Nooo I didnt copy paste the InMemoryQueue tests 🪿
| export class InMemoryQueue extends Queue {} | ||
|
|
||
| export type LocalStorageQueueOptions = QueueOptions & { | ||
| systemCode: string; |
There was a problem hiding this comment.
thought: I wonder whether this should be a generic name other than system code so that it's more understandable what it's used for? E.g. localStorageKey?
There was a problem hiding this comment.
But I think if its called localStorageKey, it gives the impression that you are actually passing the key. When the idea is that you pass the systemCode so the LocalStorageQueue build the key. If you think that needs more clarity, I could add a comment next to it to specify # Used to generate a local storage key ?
There was a problem hiding this comment.
Ah I meant that yes we pass the whole key instead of prefixing it, so the interface to the user is just setting the local storage key and it's OK if it's not the system code because, in the example of the app, we probably want a single local storage for all subsystems of the app.
E.g. digging around the existing app.ft.com localstorage (in web inspector) I think everything is under a generic o-tracking_requests. So maybe it makes sense for us to do the same, default to just cp-client-metrics-queue and allow it to be overridden.
Only add the structure, will fill the test when implementing the rest
@rowanmanning It's funny because I was actually testing things with a test html (that was very ugly). Ive cleaned it and added the skeleton of it. Let me know what you think (in particular if you think it makes sense to have it under |
| // TODO: define if we check the size in bytes of the value in local storage (if yes, we might need a helper method getStringSize(value) for ex) | ||
| // or if we just define how many events we want (like in InMemory) | ||
| MAX_STORAGE_CAPACITY = '2.5MB' |
There was a problem hiding this comment.
thought: if you made me choose I'd probably say that we just go with the number of events again because it keeps things simpler. We know roughly how large an event is and we can always add the complexity of storage size later if we encounter an issue
| // Potential helpers | ||
| #metricToString() {} | ||
|
|
||
| #stringToMetrics() {} |
There was a problem hiding this comment.
praise: makes sense, good to think about abstracting this logic as it'll probably come up a few times 👍
| @@ -0,0 +1,53 @@ | |||
| <!DOCTYPE html> | |||
There was a problem hiding this comment.
question: I don't know how this works yet, maybe we can chat about it if you're in the office later or tomorrow?
| export class InMemoryQueue extends Queue {} | ||
|
|
||
| export type LocalStorageQueueOptions = QueueOptions & { | ||
| systemCode: string; |
There was a problem hiding this comment.
Ah I meant that yes we pass the whole key instead of prefixing it, so the interface to the user is just setting the local storage key and it's OK if it's not the system code because, in the example of the app, we probably want a single local storage for all subsystems of the app.
E.g. digging around the existing app.ft.com localstorage (in web inspector) I think everything is under a generic o-tracking_requests. So maybe it makes sense for us to do the same, default to just cp-client-metrics-queue and allow it to be overridden.
2 participants
What
Create specs for LocalStorageQueue.
Why
We try to make as explicit as possible what the LocalStorageQueue will look like before implementing it.
Because the types are quite close to InMemoryQueue, we are using the unit tests (and some more details) as a contract definition.
Like this, we can discuss if we are happy with this before implementation, or if anything might be missing.