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

feat(gatsby): Rewrite resource loading - retry mechanism #14889

Merged
merged 22 commits into from
Jun 28, 2019
Merged

feat(gatsby): Rewrite resource loading - retry mechanism #14889

merged 22 commits into from
Jun 28, 2019

Conversation

Moocar
Copy link
Contributor

@Moocar Moocar commented Jun 18, 2019

I won't be able to work on this for another week or so. Posting here in case anyone wants to take a stab at finishing it. Also a good opportunity to get feedback on the actual solution.

Description

Rewrite of loader.js. Goals are:

  • Handle case where the XmlHttpRequest.readyState === 4, and statusCode === 0
  • If network error, or 500 error, or statusCode === 0, then retry 3 times before giving up
  • Clean up code to remove ad hoc "if in development do this, otherwise do this". Solved with polymorphism. Not perfect, but hopefully easier to extend.
  • Make loader instantiable. Reduce ad-hoc global state.
  • Remove separate 404 functions. e.g loadSyncOr404.
  • enable future change where cached page-data can be expired. We could for instance add a "createdAt" field to entries in the loader.pageDb, and periodically expire them, or lazily re-request.

Still TODO

  • get tests working for production-runtime and develop-runtime end to end tests
  • This is critical core functionality, and it has been rewritten, so MOAR TESTING!
  • More comments/documentation

Things I'm not happy with

  • Circular dependency between api-runner and loader. So we have to contruct both, then set the api-runner onto the loader. This is how it's always been, but it would be nice to fix. Unfortunately, to do so, we'd have to make the api-runner constructable, which would require modifications to a large part of the codebase
  • loader itself is still a global. Even though I've made it instantiable, we still need a global exported var since passing it everywhere would require change to too much of the codebase. Maybe one day
  • I've never been a fan of EnsureResources, specifically how it uses the React lifecycle functions to cause loader retries. I tried removing it, but found out it's necessary to support forwards/back functionality after a refresh. It would be great to trigger resource loading directly off window.history events, possibly even removing Reach Router entirely. Maybe one day :)

Related Issues

@KyleAMathews KyleAMathews mentioned this pull request Jun 19, 2019
Closed
wardpeet
wardpeet reviewed Jun 20, 2019
View reviewed changes
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

I've updated the PR to be in a working state ^^. Well done @Moocar this had to be super hard 😛. We probably want to add some more tests & cleanup a bit like you mentioned. We can give it a spin on gatsbyjs.org for the meantime

packages/gatsby/cache-dir/navigation.js Show resolved Hide resolved
packages/gatsby/cache-dir/loader.js Outdated Show resolved Hide resolved
@wardpeet wardpeet force-pushed the rewrite-resource-loading branch from f4a9731 to a4daff1 Compare June 20, 2019 14:54
@wardpeet wardpeet mentioned this pull request Jun 20, 2019
Merged
@Moocar
Copy link
Contributor Author

Moocar commented Jun 21, 2019

Thanks for continuing the work on this @wardpeet! Much appreciated.

@KyleAMathews
Copy link
Contributor

I think we've cracked the "Missing Resource for '/'" problem! Looking at sentry, this error has basically dropped off and the few that are still happening came from older releases (so people on service workers) https://sentry.io/organizations/gatsby-inc-9z/issues/1054226255/tags/release/

This seems ready to go.

@wardpeet wardpeet self-assigned this Jun 26, 2019
@iamskar
Copy link

iamskar commented Jun 27, 2019

Great stuff !! Do we have a timeline on when this will be merged to master?

@wardpeet
Copy link
Contributor

Writing tests and than we can merge. If it's not today than it will be tomorrow. I'll try my best to finish it today

@wardpeet wardpeet force-pushed the rewrite-resource-loading branch from 438268c to 90c5813 Compare June 28, 2019 08:43
@wardpeet wardpeet marked this pull request as ready for review June 28, 2019 09:11
@wardpeet wardpeet requested a review from a team as a code owner June 28, 2019 09:11
@m-allanson
Copy link
Contributor

🎉

@wardpeet looks like there are some merge conflicts to fix up?

@wardpeet wardpeet changed the title Rewrite resource loading feat(gatsby): Rewrite resource loading - retry mechanism Jun 28, 2019
KyleAMathews
KyleAMathews approved these changes Jun 28, 2019
View reviewed changes
Copy link
Contributor

@KyleAMathews KyleAMathews left a comment

Choose a reason for hiding this comment

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

Awesome work @Moocar & @wardpeet! This makes our runtime a lot more robust and adds a ton of tests which will help ensure continual high quality moving forward.

One note of worry — this does increase our default JS bundle by 2kb — with this PR, we've grown by 12kb since the launch of v2 to 66kb from 54kb — I'm guessing a lot of that comes from the recent PR to start compiling node_modules #14111

We should start tracking our default bundle size and do some code golfing to clean things up

@KyleAMathews KyleAMathews merged commit 68e15e7 into gatsbyjs:master Jun 28, 2019
@KyleAMathews
Copy link
Contributor 

Successfully published:
 - gatsby-cli@2.7.4
 - gatsby@2.11.1

@AlexFoster009
Copy link

Im getting a 403 hosting my site with S3 / cloudfront. Its adding an extra / before page-data.json causing issues. Works as expected on localhost:8000 but on my cloudfront distribution its adding a slash.

local: http://localhost:8000/page-data/apply-now/page-data.json
cloudfront: https://d75attx91ridx.cloudfront.net/page-data//page-data.json

as you can see there is a missing bit there. Anyone have a solution? im also not sure if this is the correct place to post this, if it isn't could someone point me into the right direction?

@wardpeet
Copy link
Contributor

wardpeet commented Jul 3, 2019

Seems like your login page uses gatsby-link where it should be just a regular a tag. Gatsby-link only works for gatsby routes.

image

johno pushed a commit to johno/gatsby that referenced this pull request Jul 17, 2019
@Moocar @johno
feat(gatsby): Rewrite resource loading - retry mechanism (gatsbyjs#14889
e8b2bcc 
)

* first stab

* ordering of requires

* working in production

* make page-changer like ensure-resources

* production-app working

* adding status checks on pageResources when requests fail

* quickfix to check if preload data works on e2e | discard later

* fix e2e-tests - need some refactoring though

* add extra pageData cache map

* fallback to dev 404 page if no 404 can be found

* add unit tests for loadPageDataJson

* Fix 404 error handling

* add unit tests for loadPage

* Add unit tests for prefetch

* consistent jest funcs

* fix tests?

* upgrade babel-preset-env to include promise.finally

* include e7 promise finally

* Revert "upgrade babel-preset-env to include promise.finally"

This reverts commit f1aecc8.

* fix client only paths

* use realpath
@jaredsilver jaredsilver mentioned this pull request Jan 7, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wardpeet wardpeet wardpeet left review comments

@KyleAMathews KyleAMathews KyleAMathews approved these changes

Assignees

@wardpeet wardpeet

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Moocar @KyleAMathews @iamskar @wardpeet @m-allanson @AlexFoster009