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

Offline dev environment #20688

Merged
merged 5 commits into from Dec 19, 2017

Conversation

Projects
None yet
6 participants
@dmsnell
Copy link
Contributor

dmsnell commented Dec 9, 2017

Use to prime a request cache, save it, then operate within Calypso out of that cache offline

See README for explanation and usage instructions.

This should only work in a local dev environment and in no other environments.

Testing

Load Calypso with the ?wpcom_priming=1 query string flag. Play around, visit various pages, open settings, open a post in the editor, view different Reader feeds, make a settings change, etc…

Open the dev tools console and run saveRequests(). Store the file in the project root directory as cached-requests.json.

Turn off your network connection then reload Calypso this time with ?wpcom_offline=1. You should b able to do the same things you did during the priming run.

Reload Calypso without any of the flags and it should work as normal.

@matticbot

This comment has been minimized.

@dmsnell dmsnell requested review from gwwar and samouri Dec 10, 2017

@dmsnell dmsnell force-pushed the offline/track-requests branch 2 times, most recently from be3e545 to 41df833 Dec 10, 2017

@dmsnell dmsnell changed the title Offline/track requests Offline dev environment Dec 11, 2017

@@ -52,3 +52,5 @@ env-config.sh
/calypso-strings.pot
cover.html
.test.log

cached-requests.json

This comment has been minimized.

@sirreal

sirreal Dec 11, 2017

Contributor

Add to .dockerignore too please!

This comment has been minimized.

@dmsnell

dmsnell Dec 11, 2017

Contributor

added in 0c4844e2fd

@dmsnell dmsnell force-pushed the offline/track-requests branch from 41df833 to 3fbf573 Dec 11, 2017

@kwight

This comment has been minimized.

Copy link
Member

kwight commented Dec 11, 2017

Change isListening to true to prime the cache.

Do you mean load http://calypso.localhost:3000/?wpcom_priming=1 according to the README? Can't find a reference to isListening in the PR.

Hm, looking further, the query param to use is actually wpcom_prime_cache.

@dmsnell dmsnell force-pushed the offline/track-requests branch from 3fbf573 to 4ee79c1 Dec 11, 2017

@dmsnell

This comment has been minimized.

Copy link
Contributor

dmsnell commented Dec 11, 2017

Thanks for those comments @kwight! I'm embarrassed that the README is out of date to begin with!

Updated in 4ee79c1

@kwight

This comment has been minimized.

Copy link
Member

kwight commented Dec 12, 2017

OK, I was able to get this going and it works well! It's awkward dealing with manually creating and copying over the cache file, but otherwise, I love it (and understand it's a jumping-off point) 👍


const readCache = () => {
try {
return require( '../../../../cached-requests.json' );

This comment has been minimized.

@samouri

samouri Dec 17, 2017

Contributor

why not start from the node root instead. I think this could be require( 'cached-requests.json' )

This comment has been minimized.

@dmsnell

dmsnell Dec 17, 2017

Contributor

oh that's probably right!

This comment has been minimized.

@dmsnell

dmsnell Dec 18, 2017

Contributor

de-relatived in 9107268

};

export const makeOffline = wpcom => {
const location = window.location;

This comment has been minimized.

@samouri

samouri Dec 17, 2017

Contributor

nit, but what do you think about:

const queryParams = new URLSearchParams( locations.search );
const primingRequested = queryParams.has( 'wpcom_priming' );
...

This comment has been minimized.

@dmsnell

dmsnell Dec 17, 2017

Contributor

that's cool. no IE support but probably worth it. in this case I really don't care about any spec as it seems quite reasonable to just search for that literal string.

think anyone developing in IE will mind? maybe I'll update it.

This comment has been minimized.

@nylen

nylen Dec 18, 2017

Contributor

How about using feature flags for this? If you do

config.isEnabled( 'offline/wpcom-priming' )

then ?flags=offline/wpcom-priming will work for free.

If that's not desirable for some reason, I would just keep the regex, and maybe make it more general to not require the =1 part (/[&?]wpcom_priming(=1|=true|&|$)/ for example).

This comment has been minimized.

@dmsnell

dmsnell Dec 18, 2017

Contributor

socked IE in 9107268

callback( ...stored );
}

return new XMLHttpRequest();

This comment has been minimized.

@samouri

samouri Dec 17, 2017

Contributor

whats this for?

This comment has been minimized.

@dmsnell

dmsnell Dec 18, 2017

Contributor

it's the interface for wpcom.js. some consumers rely on being able to set properties on the XHR itself. In this case, those will likely end up doing nothing, but I think the ones that exist mainly are there for error-handling, which theoretically won't matter here, but it will break if they don't get the XHR

This comment has been minimized.

@dmsnell

dmsnell Dec 18, 2017

Contributor

annotated in 9107268

}

// eslint-disable-next-line no-console
primingRequested && console.log( 'Priming wpcom request cache' );

This comment has been minimized.

@samouri

samouri Dec 17, 2017

Contributor

should also skip printing this if offline is requested since you can only do one or the other

This comment has been minimized.

@dmsnell

dmsnell Dec 18, 2017

Contributor

skipped in 9107268

@samouri

This comment has been minimized.

Copy link
Contributor

samouri commented Dec 17, 2017

Long term vision-wise, I could see us doing some pretty cool things with this. Two ideas right off the bat would be:

  1. daily job that runs through tons of basic operations in Calypso and commits an updated version of saved-requests to the repo
  2. auto-generations of type-data
@samouri
Copy link
Contributor

samouri left a comment

Using this was a bit magical. Seeing Calypso work offline was cool as heck.

I personally never work offline -- a dysfunctional npm turns me off. If this solves a problem for you, lets 🚢


if ( undefined === callback ) {
if ( undefined === stored ) {
return Promise.reject( new Error( 'Network inaccessible' ) );

This comment has been minimized.

@nylen

nylen Dec 18, 2017

Contributor

Here and below, this error message could include something about offline requests. "Network inaccessible and request for [path] not cached."

This comment has been minimized.

@dmsnell

dmsnell Dec 18, 2017

Contributor

good point - do you think this would be a good warrant for a console message as well? maybe a developer doesn't realize it wasn't primed?

This comment has been minimized.

@nylen

nylen Dec 18, 2017

Contributor

Sure, this would also keep the error message concise, while providing all the relevant information somewhere. +1 for a console message like "Request [URL] was not found in offline requests cache."

This comment has been minimized.

@dmsnell

dmsnell Dec 18, 2017

Contributor

in 9107268 I added a console message and changed the error text

@nylen

This comment has been minimized.

Copy link
Contributor

nylen commented Dec 18, 2017

This looks really nice, and like something we could get a lot of use out of.

The main Object.defineProperty( wpcom, 'request', ... ) block seems like the most fragile part - what would it take to add tests for this functionality, whether now or later?

Another longer-term use case could be something like a "local e2e test suite" that doesn't touch production data.

@dmsnell

This comment has been minimized.

Copy link
Contributor

dmsnell commented Dec 18, 2017

most fragile part - what would it take to add tests for this functionality, whether now or later?

not sure @nylen - we could add a custom valueOf or toString method to it but I think that would be janky. maybe we could add a property to request itself.

most of our wpcom stuff works this way, but I'm not a super fan of that either.

in this PR I think the most important thing to verify is that this code doesn't load when we don't want it to, which is anytime not in local dev as well as any time we aren't asking for it in the query string

dmsnell added some commits Dec 6, 2017

Provide offline wpcom.js mode
The purpose of this module is to facilitate offline-development of Calypso.
Under normal operations Calypso requires a working connection to the internet
in order to boot. This can be frustrating to those wanting to work on it while
on a plane, while hiking in the mountains, while waiting for dinner at a
restaurant without WiFi available, while driving, or in many other offline scenarios.

Being able to develop the product while offline brings a number of advantages to
the developer workflow and can improve our ability to deterministically test and
automate important metrics.

Although this module limits its scope to requests made through `wpcom.js`, it enables
a workable offline environment in which to test.

@dmsnell dmsnell force-pushed the offline/track-requests branch from 34b76e8 to 9107268 Dec 18, 2017

} else {
callback( ...stored );
}

// make sure we still return an XHR
// this is the expected behavior of wpcom.js requestk

This comment has been minimized.

@samouri

samouri Dec 18, 2017

Contributor

small typo. requestk

This comment has been minimized.

@dmsnell

dmsnell Dec 18, 2017

Contributor

typo, k?

This comment has been minimized.

@dmsnell

dmsnell Dec 18, 2017

Contributor

small fix in 127712a

dmsnell added some commits Dec 18, 2017

@dmsnell dmsnell merged commit bbec0b7 into master Dec 19, 2017

1 check was pending

ci/circleci CircleCI is running your tests
Details

@dmsnell dmsnell deleted the offline/track-requests branch Dec 19, 2017

dmsnell added a commit that referenced this pull request Dec 19, 2017

Offline: Add default empty cached requests file
Although there were no actual breakages, with the introduction of #20688
we started getting a warning from webpack that `cached-requests.json`
didn't exist, if indeed there were no cache file already.

This commit just makes sure that file exists so that there's no warning.

dmsnell added a commit that referenced this pull request Dec 19, 2017

Offline: Add default empty cached requests file (#20934)
Although there were no actual breakages, with the introduction of #20688
we started getting a warning from webpack that `cached-requests.json`
didn't exist, if indeed there were no cache file already.

This commit just makes sure that file exists so that there's no warning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment