Skip to content

Support User: Bypass localForage while active to avoid conflicts across sessions #3977

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 24, 2016

Conversation

jordwest
Copy link
Contributor

Bypasses localForage while support user is active. This allows sync-handler and other persistent data stores using localForage to use a 'sandboxed' temporary in-memory store while support user is active, avoiding conflicts with the original user's persisted data. This doesn't cover instances of direct localStorage usage - see #3941 which covers that.

The goal is to eventually move all direct localStorage usage to localForage.


This still needs some more testing, but overall it's working.

localforage-bypass.js was copied and modified from a localForage testing module; we can likely cut quite a bit of cruft out of it as a lot of the logic relates to test facilitation. (cruft removed in 4c39e79)

Things to test:

  • With support-user feature flag disabled
  • With wpcom_user_bootstrap enabled
  • Running in non-development environment
  • Running in development
  • Chrome 49
  • Firefox
  • Safari
  • IE

cc @retrofox @gwwar

@jordwest jordwest added [Status] In Progress [Feature Group] Support All things related to WordPress.com customer support. labels Mar 11, 2016
@jordwest jordwest force-pushed the update/support-user/bypass-localforage branch 2 times, most recently from dcdfe89 to ef62496 Compare March 15, 2016 06:27
@jordwest
Copy link
Contributor Author

@gwwar After a lot of trial and error, what we chatted about earlier worked 😃 Would you mind taking a look at this solution? Basically to solve the issues with the fact that localForage.defineDriver is async:

  • The build-env.*.js and other Calypso specific bundles are not loaded on boot. Instead we make sure support user has done any pre-boot async tasks (in this case running localForage.defineDriver) before loading the Calypso bundle. This only happens when the query string is set:
  • A ?support_user=true query string is passed to inform the server that this will be a support user session. This allows the server to leave out any user specific info including the user bootstrap and to inject a special pre-boot sequence.
  • localforage package was added to the vendor bundle so that the same instance is shared by both the support user pre-boot bundle and the main Calypso bundles.

After this change, it should even be possible to remove some of the changes introduced in #3879, as the Redux store would no longer need to know about the support user state. Instead, because initialReduxState is undefined and any attempts to save/load from localForage will be bypassed, the Redux store should function as expected without being aware that it's sandboxed.

@jordwest jordwest added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Mar 15, 2016
// original user/vice versa.
// Copied with modifications from localForage source: localforage/test/dummyStorageDriver.js

var dummyStorage = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const

@gwwar
Copy link
Contributor

gwwar commented Mar 15, 2016

👍 Nice approach. I'll clean up #3879 after this lands. I verified that we write to a dummy driver when we switch users.

We may want some more 👀 for the bundle updates. Maybe @mtias or @aduth?

@jordwest jordwest force-pushed the update/support-user/bypass-localforage branch 2 times, most recently from 4d1dc6c to b582f51 Compare March 16, 2016 05:55
@jordwest
Copy link
Contributor Author

Thanks for the review @gwwar. I've cleaned up localforage-bypass.js and added unit tests. The PR is getting quite large now, so I've split out the new pre-boot sequence to a separate PR: #4068. Once that lands this one should reduce in size.

describe( 'length', () => {
const length = () => localForage.length();
const expectLength = ( expected ) => {
return () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be nested?

@jordwest jordwest force-pushed the update/support-user/bypass-localforage branch 2 times, most recently from d284ee5 to 2d348c9 Compare March 23, 2016 03:18
@jordwest
Copy link
Contributor Author

This now uses the changes that landed in #4114 to bypass localForage on startup. Here's an overview of how we overcame the async issue described above:

localforage_bypass

@jordwest jordwest added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Mar 23, 2016
} );
const localForagePromise = localforage.defineDriver( localforageBypass )
.then( () => {
localforage.config( config );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: my bad, failed to switch over to the other branch properly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that's weird, are you seeing a Cannot bypass localforage after initialization error in the console?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh 😄

@gwwar gwwar added [Status] Needs Author Reply [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs Author Reply labels Mar 23, 2016
@gwwar
Copy link
Contributor

gwwar commented Mar 23, 2016

giphy

@jordwest
Copy link
Contributor Author

hi5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature Group] Support All things related to WordPress.com customer support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants