Skip to content
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

feat(Page): add `setCacheEnabled(enabled)` to Page object #1609

Merged
merged 2 commits into from Feb 8, 2018

Conversation

Projects
None yet
2 participants
@MountainDrew
Copy link
Contributor

commented Dec 15, 2017

Fixes: #1556

Implements Page.disableCache(disabled) method.

@MountainDrew MountainDrew changed the title 1556 feat(Page): add `disableCache()` to Page object Dec 15, 2017

docs/api.md Outdated
@@ -593,6 +594,12 @@ If URLs are specified, only cookies for those URLs are returned.
- `secure` <[boolean]>
- returns: <[Promise]>

#### page.disableCache(disabled)

This comment has been minimized.

Copy link
@aslushnikov

aslushnikov Dec 15, 2017

Contributor

Let's name this page.setCacheEnabled to be aligned with page.setJavascriptEnabled method that we already have.

This comment has been minimized.

Copy link
@MountainDrew

MountainDrew Dec 16, 2017

Author Contributor

I have made these changes 👍

@MountainDrew MountainDrew force-pushed the MountainDrew:1556 branch from c4c512a to 8aa14a9 Dec 16, 2017

@MountainDrew MountainDrew changed the title feat(Page): add `disableCache()` to Page object feat(Page): add `setCacheEnabled()` to Page object Dec 16, 2017

@MountainDrew MountainDrew changed the title feat(Page): add `setCacheEnabled()` to Page object feat(Page): add `setCacheEnabled(enabled)` to Page object Dec 16, 2017

@aslushnikov
Copy link
Contributor

left a comment

@andrewwwcollins The fromCache/fromServiceWorker methods are just landed: #1971. Are you interesteed in landing this PR? It would be nice have.

docs/api.md Outdated
- `enabled` <[boolean]> sets the `enabled` state of the cache.
- returns: <[Promise]>

Toggles ignoring cache for each request based on the enabled state.

This comment has been minimized.

Copy link
@aslushnikov

aslushnikov Feb 5, 2018

Contributor

Can you please add:

By default, caching is enabled.

responses.push(response);
});
await page.setCacheEnabled(false);
await page.goto('https://www.chromestatus.com/features', {waitUntil: 'networkidle2'});

This comment has been minimized.

Copy link
@aslushnikov

aslushnikov Feb 5, 2018

Contributor

Can you please use one of the test/assets/cached/ assets intead of chromestatus.com website? We aim for the hermetic tests.

@MountainDrew

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2018

@aslushnikov - yes I can make these changes this evening.

I have made these changes now.

@MountainDrew MountainDrew force-pushed the MountainDrew:1556 branch 4 times, most recently from 3d67b13 to 550ea85 Feb 7, 2018

responses.push(response);
});
await page.setCacheEnabled(false);
await page.goto(server.PREFIX + '/cached/one-style.html', {waitUntil: 'networkidle2'});

This comment has been minimized.

Copy link
@aslushnikov

aslushnikov Feb 7, 2018

Contributor

two nits:

  1. Let's start from making sure it's cached, and then busting caching with this method
  2. Can we load-and-reload initially to make sure the stylesheet is cached?

This comment has been minimized.

Copy link
@MountainDrew

MountainDrew Feb 7, 2018

Author Contributor

I have made these changes now. I needed to reset the responses array twice. What do you think?

This comment has been minimized.

Copy link
@aslushnikov

aslushnikov Feb 7, 2018

Contributor

ok, I sent a comment but for some reason it didn't get posted to github.
Let me re-type it here:

I needed to reset the responses array twice. What do you think?

You can avoid this by subscribing after the page.goto. You can also save a few lines by storing resources in the map:

const responses = new Map();
page.on('response', r => responses.set(r.url().split('/').pop(), r));

await page.goto(server.PREFIX + '/cached/one-style.html');
await page.reload();
expect(responses.get('one-style.css').fromCache()).toBe(true);

await page.setCacheEnabled(false);
await page.reload();
expect(responses.get('one-style.css').fromCache()).toBe(true);

This comment has been minimized.

Copy link
@MountainDrew

MountainDrew Feb 7, 2018

Author Contributor

Ah nice, didn't think of subscribing to the event later. Could be an issue when the responses map gets larger because of no filtering due to more resources being added. But I like this for the current solution.

This comment has been minimized.

Copy link
@MountainDrew

MountainDrew Feb 7, 2018

Author Contributor

Just a note for anyone else who goes to look at the above code the second expectation should be toBe(false).

This comment has been minimized.

Copy link
@MountainDrew

MountainDrew Feb 7, 2018

Author Contributor

Have made those changes now.

@MountainDrew MountainDrew force-pushed the MountainDrew:1556 branch 3 times, most recently from 3247d00 to eff7da3 Feb 7, 2018

@MountainDrew

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2018

@aslushnikov - is the appveyor continuous-integration required for this PR? I have had it passing before it just seems a bit flakey. I can rerun them if you think necessary.

MountainDrew added some commits Dec 15, 2017

feat(Page): introudce `setCacheEnabled(enabled)` method on Page object
This change adds in the `Page.setCacheEnabled(enabled)` method to toggle ignoring cache for each request.

@MountainDrew MountainDrew force-pushed the MountainDrew:1556 branch from eff7da3 to 73b726c Feb 7, 2018

@aslushnikov

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2018

@aslushnikov - is the appveyor continuous-integration required for this PR? I have had it passing before it just seems a bit flakey. I can rerun them if you think necessary.

@andrewwwcollins appveyour is very flaky for us; this has nothing to do with your change.

Thank you for you time.

@aslushnikov aslushnikov merged commit ac1b9a0 into GoogleChrome:master Feb 8, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details

brigand added a commit to brigand/puppeteer that referenced this pull request Feb 9, 2018

feat(Page): add `setCacheEnabled(enabled)` to Page object (GoogleChro…
…me#1609)

This change adds in the `Page.setCacheEnabled(enabled)` method to toggle ignoring cache for each request.

Fixes GoogleChrome#1556.

MercifulCode added a commit to MercifulCode/puppeteer that referenced this pull request Feb 9, 2018

docs: Spelling and Markdown Consistency
- Adding missing language tags to markdown code blocks.
- Switching `npm` to `yarn` in README to be consistent with what repo is using locally/with travis.

Removed unneeded  and  for yarn command per code review

Also removing  from travis and appveyor yarn invocations

Fixed merge conflicts and yarn references

Removed yarn.lock and added language to code fence

Cheeky capital letter

Touched up numbering (and also switched git email account due to CLA mis-match)

feat(Page): add `setCacheEnabled(enabled)` to Page object (GoogleChrome#1609)

This change adds in the `Page.setCacheEnabled(enabled)` method to toggle ignoring cache for each request.

Fixes GoogleChrome#1556.

docs(CONTRIBUTING): update contributing.md (GoogleChrome#1973)

@MountainDrew MountainDrew deleted the MountainDrew:1556 branch Feb 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.