Skip to content
This repository has been archived by the owner on Oct 6, 2022. It is now read-only.

Implement a new typescript version that uses Puppeteer #192

Merged
merged 10 commits into from Jul 17, 2018
Merged

Conversation

samuelli
Copy link
Contributor

No description provided.

@samuelli samuelli requested a review from aomarks July 16, 2018 21:43
src/renderer.ts Outdated
* Injects a <base> tag which allows other resources to load. This
* has no effect on serialised output, but allows it to verify render
* quality.
* @param {string} url - Requested URL to set as the base.
Copy link
Collaborator

Choose a reason for hiding this comment

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

can omit the {type} since it's already in the typescript signature

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

src/renderer.ts Outdated

// Navigate to page.
const response =
await page.goto(url, {waitUntil: 'networkidle0'}).catch(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why networkidle0 vs 1 etc.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a puppeteer option to wait until there are no outstanding network requests.

src/renderer.ts Outdated
// Navigate to page.
const response =
await page.goto(url, {waitUntil: 'networkidle0'}).catch(() => {
return undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a comment about why it's ok to ignore exceptions here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comment. Not really ignoring though, captured in the next if if statement.

const newStatusCode =
await page
.$eval(
'meta[name="render:status_code"]',
Copy link
Collaborator

Choose a reason for hiding this comment

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

comment about what's going on here with this meta tag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

src/renderer.ts Outdated
'meta[name="render:status_code"]',
(element) => parseInt(element.getAttribute('content') || ''))
.catch(() => undefined);
// Treat 304 Not Modified as 200 OK.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Presumably you have the body here because pupeteer gives you the cached response? Just note that in the comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean; will update comment. Chrome will give you a 304 when you revisit the site and its using browser cache.

src/renderer.ts Outdated
status: number; content: string;
}

export class Renderer {
Copy link
Collaborator

Choose a reason for hiding this comment

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

brief description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

}
}

restricted(href: string): boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

const app = express();
app.use(express.static(path.resolve(__dirname, '../../test-resources')));

// const appInstances:express.Express[] = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops fixed.

await rendertron.initialize(false);
await app.listen(1234);
});
// async function createServer() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@samuelli
Copy link
Contributor Author

@aomarks PTAL.

Copy link
Collaborator

@aomarks aomarks left a comment

Choose a reason for hiding this comment

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

nice

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants