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

Asynchronous render #646

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Asynchronous render #646

wants to merge 6 commits into from

Conversation

diffcunha
Copy link

Fixes #645

@toddself
Copy link
Contributor

This code requires that any browser or node instance that doesn't support async be compiled using babel and the polyfill being included.

Being that we still have support enabled for IE 11 and Safari (browser which represent more than 1% of usage), this adds a lot of weight to choo. (babel-poylfill is around 85k depending on compression which is MUCH larger than choo)

@diffcunha
Copy link
Author

I just realised that, I'm removing async/await now

@toddself
Copy link
Contributor

Also, check out my comments in #645 -- there are other ways to do this without needing to change how choo works

@diffcunha
Copy link
Author

I forgot to mention but this change is also backward compatible, normal sync rendering will still work

@toddself
Copy link
Contributor

Promise still needs to be shimmed for IE 11 (so we'd likely also need to include a promises implementation in core).

@diffcunha
Copy link
Author

Does it have to be included in core? It could be up to the developer to decide if she wants it or not and which shim to use. Otherwise we could use es6-promise in core, it takes 2.4 KB gziped

@YerkoPalma
Copy link
Member

I think this whole PR could fit for a store. Since the the .use function expose the app instance, you can change whatever you want there, maybe is a good idea to have it as astore and make some tests and bench on top of it.

@diffcunha
Copy link
Author

diffcunha commented Mar 15, 2018

@YerkoPalma that's actually a good idea. I could definitely do it but, there is a recent change to delegate store initialisation (#638), it makes it harder since the patching would only be applied after calling start or toString and I also need to patch those 2

@diffcunha diffcunha mentioned this pull request Mar 16, 2018
@diffcunha
Copy link
Author

@toddself I updated the PR so that the a Promise shim is only required if the developer wants to go with async rendering. If there are no promises being passed to html, everything works just like before.

@mcollina
Copy link

I really like this implementation, as it brings the best of both worlds and it's not a breaking change.
I would add some more tests with mixed values (promises, literals) as well.

@Powersource
Copy link

Any update on this? :)

Anything a noob contributor (me lol) could help out with?

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.

Asynchronous render
5 participants