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

Support Helmet v5, add include/exclude support, fix express 200.html rendering, only crawl potential html pages #24

Closed
wants to merge 11 commits into from

Conversation

CodyRay
Copy link
Contributor

@CodyRay CodyRay commented May 11, 2017

Apologies, it seems I have put a lot into this PR. I was kind of on a roll though. I tried to test as many configurations as I could and it seems pretty stable.

Note: I went ahead and renamed the path option to include since it wasnt documented and it appears react-snapshot hasnt been released yet. But I can make that backward compatible as well...

Fixes #17
Fixes #19
Incorporated #23 and #22
Fixes #15 and #16 in a few more cases, also handles includes and excludes for basenames
Closes #11

Also various other things I found

  • Only crawl potential html pages (not links to .jpg files for instance)
  • Release resources so that the server can gracefully shutdown
  • Removed hardcoded port
  • Updated readme

geelen and others added 9 commits April 27, 2017 22:35
Also various other bugs I found
 * Fixes #19 Saving subroutes
 * Clean up path config option
 * Add support for excluding files
 * Fixes problems with a basename (related to #15, #16)
 * package.json only read once now
 * only crawl potential html pages (not links to .jpg files for instance)
 * Fix Server so that it renders 200.html if a file was not found
 * Release resources so that the server can gracefully shutdown
 * Removed hardcoded port
 * Updated readme
…into haroldhues-master

Conflicts:
	package.json
	src/Crawler.js
	src/Server.js
	src/cli.js
	src/snapshot.js
@geelen
Copy link
Owner

geelen commented May 19, 2017

Hey! Thanks for this! I'd actually done a little bit of work myself on a few of those, but hadn't finished and merged it. I've just done so now at #26

There were a few merge conflicts but I've fixed them and pushed the code on the haroldhues-master branch (also pointed #27 at it). I'm seeing an error with ext.pathname in my local testing, can you take a look and see if maybe something got broken in the merge?

Very keen to get a new release out with these changes in the next few days. Thanks again for picking up a bunch of things at once 👌

CodyRay added 2 commits May 19, 2017 08:04
I couldn't get it to work for my use case, might be that it didn't work with  a mountpath?
@CodyRay
Copy link
Contributor Author

CodyRay commented May 19, 2017

@geelen Path problem is fixed, I could not get connect-history-api-fallback to work with my project. I didn't look into it too much, but I replaced it with a simple middleware that I believe will do the same thing. I didn't test if the proxy still works. 🙈

BTW create-react-app also supports passing an object to proxy. It might be worth handling that case as well :bowtie:

@geelen
Copy link
Owner

geelen commented May 24, 2017

Closing in favour of #27

@geelen geelen closed this May 24, 2017
@geelen
Copy link
Owner

geelen commented May 26, 2017

Boom. Finally figured out the bugs with this, and have merged & published v1.1.0

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.

2 participants