-
-
Notifications
You must be signed in to change notification settings - Fork 634
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
Feature support for async prefetch in inferno-router #1621
Conversation
"renders on initial" is a faulty test, I will fix. |
By removing async/await from browser bundled code and moving resolveLoader to inferno-server the inferno-router bundle returns to an acceptable size: Original cjs prod: 12.8kB UPDATE: Current size is 17.34 kB |
|
Update: I have working tests with <Switch> but it needs some cleanup and more test cases. |
@Havunen I have implemented the code for <Switch>
|
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
|
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. |
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) |
@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.
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. |
@Havunen I think the PR is complete now. If you want to explore how it works you can:
Visit http://localhost:3000/ for SSR and http://localhost:1234/ for client side rendering only.
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", |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WILL FIX
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 65c7d77
ts lint is reporting errors, can you check those please |
demo app is also logging warning to console:
|
I believe ParcelJS is consuming the wrong bundle. I will see if I can solve it without requiring a custom resolver. WILL INVESTIGATE |
…e it is an implementation detail
…ending renders are flushed
…e library code to verify that test fails)
…ed dev version in demo
a798ad7
to
37ba7ed
Compare
@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'); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 54581c7
There was a problem hiding this 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! :)
@Havunen tests are passing and I fixed a bunch of linting errors too. |
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.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