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.

Sign up for GitHub

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

Jump to bottom

Proxy / CDN Config Implementation #136

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Proxy / CDN Config Implementation #136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7025c31
Implementing proxy.
Feb 7, 2018
100a6d7
adding logging and fixing proxy errors.
Feb 7, 2018
5821dbf
removing console logs.
Feb 7, 2018
7cdaf87
Merge pull request #1 from InSpread/proxy-implementation
kirkchris Feb 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ const defaultOptions = {
puppeteerExecutablePath: undefined,
puppeteerIgnoreHTTPSErrors: false,
publicPath: "/",
proxy: {},
Copy link
Owner

Choose a reason for hiding this comment

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

there is proxy config in CRA which I plan eventually support, so there is a name clash. Not sure what to do about it. Otherwise PR looks fine, but haven't tested it myself

Copy link
Author

Choose a reason for hiding this comment

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

Verified in our own deploy and code works. We're using it in production now.

Any thoughts on name? I can use cdn if you think that's better?

Copy link
Owner

@stereobooster stereobooster Feb 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Three options (which I do not like)

  • proxy (good, but clashes with other one)
  • browserProxy
  • cdn
  • thirdPartyResources

There are only two hard problems in the programming...

Copy link
Author

@kirkchris kirkchris Mar 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

bah, sorry about the delay on this... I didn't have any brilliant ideas on the naming 😒 I think of those, I probably like thirdPartyResources the best. My concern with cdn is that it wouldn't always be a cdn, just any third party. What do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

You are not delaying it. It is rather me, who have not much time recently to do updates on this project. I didn't expect that you alone should come up with the name. I still have no good idea either

Copy link

Choose a reason for hiding this comment

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

pardon my stupidity, but how does this clash? i use CRA and reactSnap's config is a separate object, so reactSnap.proxy doesn't conflict with proxy with CRA uses. though if it does, what do you think of snapProxy for a name?

Copy link

@jayenashar jayenashar May 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

after trying to write tests for this in #179, this seems very different to CRA's proxy. this would be better called cdn to avoid confusion.

minifyCss: {},
minifyHtml: {
collapseBooleanAttributes: true,
Expand Down
32 changes: 23 additions & 9 deletions src/puppeteer_utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,33 @@ const path = require("path");
const fs = require("fs");

/**
* @param {{page: Page, options: {skipThirdPartyRequests: true}, basePath: string }} opt
* @param {{page: Page, options: {skipThirdPartyRequests: true, proxy: {}}, basePath: string }} opt
* @return {Promise<void>}
*/
const skipThirdPartyRequests = async opt => {
const handleThirdPartyRequests = async opt => {
const { page, options, basePath } = opt;
if (!options.skipThirdPartyRequests) return;
if (!options.skipThirdPartyRequests || !options.proxy) return;
await page.setRequestInterception(true);
page.on("request", request => {
if (request.url().startsWith(basePath)) {
request.continue();
} else {
if (options.proxy) {
for (proxyUrl in options.proxy) {
if (request.url().startsWith(proxyUrl)) {
const requestChanges = {};
if (typeof options.proxy[proxyUrl] === 'string') {
requestChanges.url = request.url().replace(proxyUrl, options.proxy[proxyUrl]);
}
request.continue(requestChanges);
return;
}
}
}

if (options.skipThirdPartyRequests && !request.url().startsWith(basePath)) {
request.abort();
return;
}

request.continue();
});
};

Expand Down Expand Up @@ -164,8 +178,8 @@ const crawl = async opt => {
try {
const page = await browser.newPage();
if (options.viewport) await page.setViewport(options.viewport);
if (options.skipThirdPartyRequests)
await skipThirdPartyRequests({ page, options, basePath });
if (options.skipThirdPartyRequests || options.proxy)
await handleThirdPartyRequests({ page, options, basePath });
enableLogging({
page,
options,
Expand Down Expand Up @@ -217,7 +231,7 @@ const crawl = async opt => {
});
};

exports.skipThirdPartyRequests = skipThirdPartyRequests;
exports.handleThirdPartyRequests = handleThirdPartyRequests;
exports.enableLogging = enableLogging;
exports.getLinks = getLinks;
exports.crawl = crawl;