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

Feature support for async prefetch in inferno-router #1621

Merged
merged 72 commits into from May 13, 2023

Conversation

jhsware
Copy link
Contributor

@jhsware jhsware commented Dec 17, 2022

All tests are passing for me.

This PR adds a property loader to <Route> which will be resolved prior to performing a navigation. This allows data to be fetched from an API and the result sent to the rendered component. The behaviour mimics the loader behaviour of React Router v6.

 <Route path="/" loader={async () => {} }

For SSR you can run await resolveLoaders(...) on your initial application and get the results of all the invoked loaders for a given location. This can then be passed to a <StaticRouter loaderData={...} > for server side rendering and then used during rehydration in the browser by passing the same data to <BrowserRouter loaderData={...} >.

This will greatly simplify SSR with Inferno which currently requires both patience and guts.

I have added some helper methods to abstract access to data in a way that looks familiar to those who are accustomed to the hooks of react-router v6. This has the benefit of allowing us to change implementation details without breaking applications.

Reference: https://reactrouter.com/en/main/route/loader

@coveralls
Copy link
Collaborator

coveralls commented Dec 17, 2022

Coverage Status

Coverage: 93.357% (-0.01%) from 93.367% when pulling e5b0673 on jhsware:feature-router-async-loader into 43437ba on infernojs:master.

@jhsware
Copy link
Contributor Author

jhsware commented Dec 18, 2022

"renders on initial" is a faulty test, I will fix.

@jhsware
Copy link
Contributor Author

jhsware commented Dec 19, 2022

By removing async/await from browser bundled code and moving resolveLoader to inferno-server the inferno-router bundle returns to an acceptable size:
│ inferno-router │ 39.77 kB/7.77 kB │ 14.31 kB/4.37 kB │ 31.76 kB/6.84 kB │ 35.59 kB/7.47 kB │ 14.16 kB/4.28 kB │

Original cjs prod: 12.8kB
New cjs prod with async loader support: 14.16 kB

UPDATE: Current size is 17.34 kB

@jhsware
Copy link
Contributor Author

jhsware commented Dec 19, 2022

  • Consider renaming loaderData to initialData to match initialEntries.
  • Pass params to async loader
  • Pass request to async loader
  • Add tests with params
  • Add tests with request
  • Implement support in <Switch >

@jhsware
Copy link
Contributor Author

jhsware commented Dec 21, 2022

Update: I have working tests with <Switch> but it needs some cleanup and more test cases.

@jhsware
Copy link
Contributor Author

jhsware commented Dec 22, 2022

@Havunen I have implemented the code for <Switch>

  • refactored function extractMatchFromChildren (readability, size but adds one extra function call when resolving switch)
  • all tests are passing
  • STARTED: preferably need more test cases
  • DONE: there is room to DRY up Route and Switch
  • DONE: I need to look at the timing to make sure we don't remove the current route before loader has been resolved
  • TODO: also need to look at support for progress bar/loading state (could be released without)

@jhsware
Copy link
Contributor Author

jhsware commented Dec 23, 2022

I have added a demo app so we can visually check that routing behaves as intended. It appears that the outgoing route is removed before data has been returned. I need to investigate.

NOTE: Requires Nodejs >=18

  • solve timing issue when switching route
  • perform SSR in demo
  • use hydrate when loading SSR page in demo

@jhsware
Copy link
Contributor Author

jhsware commented Dec 23, 2022

I have resolved the timing issue by giving Router responsibility for invoking loaders (I was thrown off by the location matching being done in each Route and placed the loader calls there first). This will also help implementing progress tracking. Broke some stuff but I am hoping it only requires some minor fixes.

@jhsware
Copy link
Contributor Author

jhsware commented Dec 23, 2022

All tests are passing and demo works as expected. I'll try to clean up the code, fix some naming inconsistencies and address the items in #1621 (comment)

@jhsware
Copy link
Contributor Author

jhsware commented Dec 26, 2022

@Havunen I think we might have the feature set in place now. I believe I have implemented all the important features. Let me know if this makes sense. A request can be aborted, but I haven't implemented form submissions. It requires more work and form submission can be handled by the component without inferno-router being involved so I think we can live without it for starters.

I need to check the SSR-stuff once more because I updated resolveLoaders.ts, but it will be pretty straight forward.

DONE: I copied most of the code for providing a request to the loader from react-router but I think it could be simplified using the URL class. I am guessing they did it this way to avoid depending on a DOM-like environment. But before I simplify I would like to know that you are at least moderately happy with what we have so far.

@jhsware
Copy link
Contributor Author

jhsware commented Dec 28, 2022

@Havunen I think the PR is complete now. If you want to explore how it works you can:

npm run build
cd demo/inferno-router-demo 
npm run dev

Visit http://localhost:3000/ for SSR and http://localhost:1234/ for client side rendering only.

  • requires Node 18 due to SSR-implementation
  • supports transpilation of SSR-bundle on start up with restart on change using ParcelJS + ts-node
  • I have set an artificial delay to show that the old view stays until the new is ready

When you are happy, I will update the docs:

I also intend to add SSR-templates which would make creating Inferno SSR-apps using Node 18 a breeze:

"browserslist": "> 0.5%, last 2 versions, not dead",
"scripts": {
"dev": "npm-run-all -l --parallel dev:*",
"dev:frontend": "FORCE_COLOR=1 parcel",
Copy link
Member

Choose a reason for hiding this comment

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

these scripts does not run on windows due to different syntax of environment variables than unix, please use cross-env

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WILL FIX

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 in 65c7d77

@Havunen
Copy link
Member

Havunen commented Jan 2, 2023

ts lint is reporting errors, can you check those please

@Havunen
Copy link
Member

Havunen commented Jan 2, 2023

demo app is also logging warning to console:

You are running production build of Inferno in development mode. Use dev:module entry point.

@Havunen
Copy link
Member

Havunen commented Jan 2, 2023

I noticed that if the user clicks page two in the demo app multiple times quickly and then clicks page one after it, it does not move to the page one but stays on page two. It might be a bug in the demo app or in the library

image

It could be the abort logic that needs fixing?

@jhsware
Copy link
Contributor Author

jhsware commented Jan 2, 2023

demo app is also logging warning to console:

You are running production build of Inferno in development mode. Use dev:module entry point.

I believe ParcelJS is consuming the wrong bundle. I will see if I can solve it without requiring a custom resolver.

WILL INVESTIGATE

@jhsware jhsware force-pushed the feature-router-async-loader branch from a798ad7 to 37ba7ed Compare April 26, 2023 15:49
@Havunen
Copy link
Member

Havunen commented May 9, 2023

@jhsware the build is failing

render(<BrowserRouter history={history} />, node);

expect(console.error).toHaveBeenCalledTimes(1);

// browser only?
expect(console.error.calls.argsFor(0)[0]).toContain('<BrowserRouter> ignores the history prop');
expect((console.error as any).calls.argsFor(0)[0]).toContain('<BrowserRouter> ignores the history prop');
Copy link
Member

Choose a reason for hiding this comment

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

putting the result of spyOn(console, 'error'); to variable corrects the typing so "any" is not needed

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 in 54581c7

Copy link
Member

@Havunen Havunen left a comment

Choose a reason for hiding this comment

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

build and tests should pass, otherwise LGTM! :)

@jhsware
Copy link
Contributor Author

jhsware commented May 13, 2023

@Havunen tests are passing and I fixed a bunch of linting errors too.

@Havunen Havunen merged commit b1befe9 into infernojs:master May 13, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants