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

Lazy Loading with Preact Router #135

Closed
wants to merge 15 commits into from
Closed

Conversation

prateekbh
Copy link
Member

Hey!
Opening this PR to track the work for Lazy Loading in Preact Router.
Adding new Component AsyncRoute which takes a prop component.
Prop component can either be a function returning a Component or returning a promise which eventually resolve in a Component.

I still have to play with Webpack and see if this works out of the box.

src/index.js Outdated
}
componentDidMount(){
const componentData = this.props.component();
if (componentData instanceof Promise) {
Copy link
Member

Choose a reason for hiding this comment

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

I usually just check for componentData && componentData.then so that any promise lib works without necessarily being global.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

src/index.js Outdated
if (componentData instanceof Promise) {
componentData.then(promiseData => {
this.setState({
componentData: h(promiseData, { url: this.props.url, matches: this.props.matches })
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: call promiseData "component" and store that in state, then invoke h() on it in render() instead of here. That will work for updates after initial loading.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

src/index.js Outdated
};
}
componentDidMount(){
const componentData = this.props.component();
Copy link
Member

Choose a reason for hiding this comment

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

Random thought (not sure what the best approach to implement it would be): we could cache the promise return value here using the promise-returning function as a key. That would work really nicely with bundle-loader?async.

Copy link
Member Author

@prateekbh prateekbh Feb 1, 2017

Choose a reason for hiding this comment

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

Ok you need to explain me this a little.
componentDidMount runs only once a lifecycle of a Component.
so 2 questions:
1.) Even if I cache when do you expect me to use it? (next time the same component gets mounted?)
2.) Caching obviously cannot be done on the scope of this as it will be destroyed when you change route, so you wanna cache at Router level or a global level in this file?

@developit
Copy link
Member

Just some random musings on there for you while I had a second to look at it. Nice stuff.

test/index.js Outdated
};
render(<AsyncRoute component={getComponent} />, containerTag);

setTimeout(() => {
Copy link
Member Author

Choose a reason for hiding this comment

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

@developit I wonder if there's a way to avoid such small timeouts... not a big fan
Without a small timeout it fails...

Copy link
Member

Choose a reason for hiding this comment

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

You can turn off async rendering:

import { options } from 'preact';

options.syncComponentUpdates = false;
options.debounceRendering = f => f();

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks a ton!

@prateekbh
Copy link
Member Author

prateekbh commented Feb 1, 2017

@developit yeah now AsyncRouter supports returning a Promise and as well as a callback style use case.
I would just only wanna know what are your thoughts about the caching thing.

P.S. Tests included with the PR :)

@prateekbh prateekbh changed the title Lazy Loading with Preact Router [DO NOT MERGE] Lazy Loading with Preact Router Feb 2, 2017
@prateekbh
Copy link
Member Author

@developit this is ready for round 2 of review

package.json Outdated
@@ -81,5 +81,8 @@
"sinon-chai": "^2.8.0",
"uglify-js": "^2.6.1",
"webpack": "^1.13.1"
},
"dependencies": {
"Promise": "^1.0.5"
Copy link
Member

Choose a reason for hiding this comment

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

should be in devDependencies

Copy link
Member Author

Choose a reason for hiding this comment

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

oops, done

@developit
Copy link
Member

Still thinking we might want to make this separate from the main bundle. There are a bunch of differing use-cases people might have, different caching semantics, etc. Might be a bit much to push a single async component on people. Thoughts?

@prateekbh
Copy link
Member Author

prateekbh commented Feb 6, 2017

Yes the idea sounds 💯 .
and I can imagine something like this.

import {AsyncRoute} from 'preact-router/addons'

For this I can add an index.js in a new addons folder and make rollup yield addons.js in dist folder.

Let me know if this sounds cool, code is ready as always 😄. will push and ping you

P.S. Anybody wanting es5 code directly would need to use preact-router/dist/addons.js

@prateekbh prateekbh mentioned this pull request Feb 6, 2017
@developit
Copy link
Member

I can't think of any other addons - might be simpler just to call it "async" and have the export be your component (preact-router/async, everything else the same).

@prateekbh
Copy link
Member Author

done :)
A totally separate folder async.
I am doing rollup for the first time, so please let me know if i did any blunder there.

@prateekbh
Copy link
Member Author

@developit, shall we go ahead with this?

@prateekbh
Copy link
Member Author

@developit
closing this PR in favor of https://github.com/prateekbh/preact-async-route.

would be awesome if we can highlight the same in README

@prateekbh prateekbh closed this Feb 16, 2017
@developit
Copy link
Member

You want to PR, or should I?

@prateekbh
Copy link
Member Author

I'll send u the updated readme

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.

None yet

2 participants