feat(AP-2173): Add persistence to context #147
Conversation
|
Kudos, SonarCloud Quality Gate passed!
|
|
Unit tests would be good too if you can add some. |
|
There's an argument to be made that these changes belong in
|
There was a problem hiding this comment.
Placing our whole context onto session and local storage concerns me as it dramatically increases the information that we store on a cookie like area -- that customers ask for information on when we go through security reviews -- so it should at least be optional
We also make this information available to chrome plugins
The other concern is that this makes the functionality specific to the client -- could be argued that is ok, given a true sdk implementation, the dev controls the lifecycle, and this code will be ignored in npm. But we've previously managed to browser specific implementations to asset manager
Also -- tests
| } | ||
|
|
||
| function mergeAllPersistedStorage(contextKey, context) { | ||
| if (window === undefined || !window.localStorage) { |
There was a problem hiding this comment.
javascript-sdk currently has no references to session or local storage. Is this check to prevent errors if we're running on node?
A warning here that persisting is doing nothing would be good -- but should this capability live in asset-manager?
There was a problem hiding this comment.
Yeah, I'll add a warning.
@robertsevernsentient I agree that we shouldn't persist the full context. This functionality allows the consumer to specify one or more keys to persist, not the full context. I can add a warning about not persisting when executing in NodeJS. I could also make the storage pluggable, but the usecases for this would be limited server-side. |
This change allows an implementer to designate context keys to persist in either local or session storage.