search

freedomjs / freedom

Embracing a distributed web
http://freedomjs.org
Apache License 2.0
512 stars 53 forks source link

Lazy module loading #310

Closed bemasc closed 8 years ago

bemasc commented 8 years ago

Prior to this change, all dependencies specified in a module's manifest were loaded and started before that module could start. With this change, the main module is loaded immediately, and each dependency is only loaded after its constructor is called. This design makes startup faster, and reduces memory usage for projects that contain optional modules.

This change does not alter any explicit API, but it does change the semantics of modules that perform actions before their constructor is called. Any such actions will now be delayed until after the first constructor call. I am not aware of any such modules currently in existence.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.4%) to 80.874% when pulling 615f8545df57aa7356339739fea0747a30773e9b on bemasc-lazy into b9c00ec4aaa0d106374c5b3948bea28ec3024247 on master.

willscott commented 8 years ago

Awesome!

What happens if a dependency fails to load? is there a test around that case worth writing?

bemasc commented 8 years ago

I have no idea ... I'm not even sure what should happen. I'm happy to add a test case, especially if you can recommend a good starting point.

willscott commented 8 years ago

if a dependency fails after startup, we call the onError handler in the parent. Currently, if it fails to start up, the parent is never launched either, and the overall freedom failure at startup is reported to the top context.

I think with this change it would make more sense for the parent to be able to register a freedom.dependency.onError(handler); which is also invoked on failure to construct / start the dependent module.

This is probably the most similar test, which is for attempting and failing to dynamically instantiate a dependency after startup: https://github.com/freedomjs/freedom/blob/master/spec/providers/coreIntegration/environment.integration.src.js#L47

bemasc commented 8 years ago

OK, I'm working on that. It's turning out to be a pretty big change, because there's no system in place for broadcasting a message from a module to all the other modules that depend on it.

bemasc commented 8 years ago

Done. PTAL. This was Not Easy.

Tests pass for me; I don't know what's up with shippable.

willscott commented 8 years ago

Awesome!

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.4%) to 80.878% when pulling 5068eece820c46eeb7e94f734a02fe853664a83c on bemasc-lazy into b9c00ec4aaa0d106374c5b3948bea28ec3024247 on master.

willscott commented 8 years ago

@soycode - when you get a chance can you sanity check that this change propagates okay down to freedom-for-chrome / freedom-for-firefox / freedom-for-node ? I think you're the most likely to have the current state of the various instantiations in your head at the moment. If all is good, feel free to bump+push new versions for all.

agallant commented 8 years ago

Acknowledged - will get to it soon, would be good to bump versions for other small fixes too.

agallant commented 8 years ago

No new test failures in firefox/chrome/node. node is passing all tests (as that's what I visited last), firefox/chrome seem to have a few new incidental failures due to whatever churn, but it seems orthogonal to this change.

Still, I may dive into them a bit and try to fix before bumping - is cutting new releases blocking anything?

willscott commented 8 years ago

Lets aim to get a bump this week. no problem if some changes you're interested in slip to the next one.

On Tue, May 3, 2016 at 1:00 PM, soycode notifications@github.com wrote:

No new test failures in firefox/chrome/node. node is passing all tests (as that's what I visited last), firefox/chrome seem to have a few new incidental failures due to whatever churn, but it seems orthogonal to this change.

Still, I may dive into them a bit and try to fix before bumping - is cutting new releases blocking anything?

— You are receiving this because you modified the open/close state. Reply to this email directly or view it on GitHub https://github.com/freedomjs/freedom/pull/310#issuecomment-216647555

agallant commented 8 years ago

Sounds good, I'll spelunk a bit in the name of green CI, but call it at some point as at least an incremental improvement.

agallant commented 8 years ago

Actually, just tested freedom proper and I'm failing 12 integration tests locally, same as on Shippable - https://app.shippable.com/runs/5727d55199e6210c008cda93

They pass on Travis, but still will want to figure this out. Does potentially look related to this change.

agallant commented 8 years ago

Filed an issue to move the discussion out of this closed thread. https://github.com/freedomjs/freedom/issues/312