Skip to content
This repository has been archived by the owner on Dec 5, 2022. It is now read-only.

Allow lazy fragment initializaiton through promise #94

Merged
merged 2 commits into from
Jan 4, 2017

Conversation

vigneshshanmugam
Copy link
Collaborator

@vigneshshanmugam vigneshshanmugam commented Jan 3, 2017

  • This is a backward compatible change

fixes #92

Copy link
Contributor

@grassator grassator left a comment

Choose a reason for hiding this comment

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

I would restructure this to have something like:

function doInit(init, callback) {
// do actual initialization and when done do callback
}
doInit(init, measureInitCost.bind(null, fragmentId, 'fragment-');

That way you can avoid multiple early returns.

// Check if the exposed function is a Promise to allow lazy rendering
// Capture initializaion cost of each fragment on the page using User Timing API if available
if (isPromise(init)) {
init.then(function(module) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that init function should rather return a promise

// check if User Timing API is supported
var isUTSupported = perf && 'mark' in perf;

function isPromise(obj) {
Copy link
Contributor

Choose a reason for hiding this comment

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

pull this out into a library function and import it

Copy link
Collaborator Author

@vigneshshanmugam vigneshshanmugam Jan 3, 2017

Choose a reason for hiding this comment

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

I would keep it simple.. will probably even inline it than keeping it as fn

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe, but it's probably something that should be covered by a test, so pulling it out into a library func is going to help with that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah i didnt do that since its not used anywhere other than this one place..

Copy link
Contributor

Choose a reason for hiding this comment

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

oki

@vigneshshanmugam vigneshshanmugam merged commit 47ed950 into master Jan 4, 2017
@vigneshshanmugam vigneshshanmugam deleted the lazy-render branch January 4, 2017 11:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to consume promises from init function of fragments
3 participants