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

WIP: Replace jsdom with chrome headless #44

Closed
wants to merge 1 commit into from

Conversation

badsyntax
Copy link

@badsyntax badsyntax commented Jul 29, 2017

I've marked this as a WIP as it's not ready to merge.

This is just an initial pass at replacing jsdom with chrome headless. Let's get a discussion going.

Thing change brings some PROS:

  • A real browser with no missing API's
    • Less bugs
  • No requirement on ReactDOMServer to render, no change required in userland to how a site is rendered.

And some CONS:

  • Google Chrome is required to be installed on the system
  • Not at all compatible with V2

Note this change also includes:

  • Basic testing using jest snapshots
  • Linter (standardjs, which seems to match the existing code style)

@badsyntax
Copy link
Author

@grahamlyus
Copy link

@badsyntax @geelen

This is great - I haven't had much success with other pre-rendering solutions. I ran into the jsdom limitations pretty quickly (localStorage) so checked out the PR.

However, I don't use create-react-app on the project I wanted to pre-render and had some other needs, so thought I'd add some thoughts:

  • my build directory is dist - this would be easy enough to deal with via a config option.

  • I want clean-ish URLs and have some nested routes, e.g. /blog and /blog/:slug, so I changed the filename pattern from ${urlPath}.html to ${urlPath}/index.html - again that could be easy via optional config.

  • I wanted to ignore any resources outside of my app (Analytics, FB SDK, etc) - it's fine to render their script tags, but loading them during the pre-render stage will cause issues with duplicate scripts, etc. because most of them are thin loader scripts (I actually also see my scripts get added twice) - chromy has blockUrls, but this is blacklist based and only support wildcards.

  • I still found the need to know that the code was running in the pre-render environment (again to prevent analytics, etc running) - chromy does not appear to userAgent config, which would be an approach to this.

  • Sitemap generation could be a cool addition (prep does this, although it's basic - tbh I don't know how important priority, changefreq, lastmodISO are in reality)

I found https://github.com/GoogleChrome/puppeteer, which allows me to set the userAgent and also intercept requests and reject any that do not start with this.baseUrl.

@badsyntax
Copy link
Author

There's been no discussion on this MR and it looks like the project has taken a different path so I'm tempted to close this.
If the author wants to integrate chrome I would definitely go for puppeteer.

@geelen
Copy link
Owner

geelen commented Aug 31, 2017

Sorry I've been super busy recently. Yes, puppeteer looks like the best of the bunch. Yes, JSDOM is not long for this world—there are too many incompatibilities with modern apps. The current thing I'm trying to figure out is whether it'll be possible to run during a deploy on Heroku or not.

Basically, this project is gradually orbiting towards a real v2 release backed by Chrome headless and with full async support. It'll probably require React 16+ due to a much more friendly rehydration API. But I'm absolutely taking this PR as the inspiration for all that work. I hope I can get it all released soon!

@badsyntax
Copy link
Author

@geelen just seen this thread about people having a bunch of issues (including with heroku). i guess they're still ironing out some kinks. either way i'm happy to update this MR with puppeteer on top of the async-16 branch if you like?

@grahamlyus
Copy link

@badsyntax @geelen To save you the trouble, I'm happy to chip in with my changes to this PR for puppeteer, either via a new PR or gist, it was quite simple.

I'm running the pre-prender locally before deploying to s3, so cannot speak to any Heroku issues.

@sebtrif
Copy link

sebtrif commented Sep 15, 2017

Happy to help however needed!
Already using this branch with react-snapshot because the final output from jsdom sometimes includes extra empty elements in my case :/

@stereobooster
Copy link

This will solve #33, #34, #50, #40

@stereobooster
Copy link

stereobooster commented Oct 1, 2017

I were using this awesome project, but because of problems with jsdom wrote small script to do snapshots. It is only 100+ LOC uses puppeteer and highland as a queue. https://github.com/stereobooster/react-snap/blob/master/index.js

@geelen
Copy link
Owner

geelen commented Nov 2, 2017

Just FYI, I think using puppeteer is definitely the right choice, but given the presence of react-snap I don't think I'll be implementing that here. I'll either build on top of or merge with that project going forward.

Apologies for not getting this merged straight away, and leaving this project lapse for a while. Closing this now in favour of #80

@geelen geelen closed this Nov 2, 2017
@badsyntax
Copy link
Author

@geelen No apology required! Thanks for your great work and ideas on this project. I think the original goal of the PR has been achieved, in a somewhat roundabout way, via react-snap using puppeteer. Hopefully there can be some collaboration around async support in the future.

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

Successfully merging this pull request may close these issues.

5 participants