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

Adds lazy-load-overview #141

Closed
wants to merge 4 commits into from

Conversation

MiguelMadero
Copy link

@MiguelMadero MiguelMadero commented Apr 26, 2016

  • Create asset-service
  • Create asset-service RFC
  • Remove low-level details to keep this only as overview and link to other RFC's or other work for additional info.
  • Add info about LOG_RESOLVER flag to the list of unresolved questions. It seems like all the route related objects are eagerly resolved if the flag is on.

cc: @stefanpenner @chadhietela @rwjblue @ebryn @dgeb

@stefanpenner
Copy link
Member

stefanpenner commented Apr 26, 2016

this seems like an ember-cli rfc?

## Loading assets

[ember-cli-bundle-loader](https://github.com/MiguelMadero/ember-cli-bundle-loader) provides a
[service](https://github.com/MiguelMadero/ember-cli-bundle-loader/blob/master/addon/services/lazy-loader.js)
Copy link
Member

Choose a reason for hiding this comment

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

I like the asset loading service idea, this is a great step for loading costly ad-hoc deps, and may benefit from a high level route api that declares its deps.

I believe an RFC for this would be on ember-cli/rfcs as it requires tight coupling to the build-pipeline to work, and i don't see it becoming part of ember.js itself for that reason. Although it should likely be shipped as part of ember-cli.

AssetService seems like a legit starting point, that I feel quite comfortable moving forward on.

Copy link
Author

Choose a reason for hiding this comment

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

I'll look into it.

it requires tight coupling to the build-pipeline

I think it makes more sense to decouple them, but let me think it through a bit more. There are many ways in which we can build assets today from CLI, engines will add one more and we have cases where we might get assets from CDNs or generated by a different asset-pipeline. I think we need a manifest (for all CLI assets, similar to what we get from AssetRev) an extension point, so users can extend it for external assets, but the service can focus on loading JS/CSS in a reliable promise-aware way that can be used by components and the router.

@stefanpenner
Copy link
Member

stefanpenner commented Apr 26, 2016

This RFC seems to go a bit deep on brand new ideas, I think it should either focus entirely on 1 of those new ideas (like the asset-service) or it should stick to the high-level narrative between the solutions.


For app code, we likely want an integration with the router, which can rely on this service to do the actual loading.

When splitting the app's JS and CSS into multiple assets related to different sectionsof an application, we want to load the code dynamically. This today poses additional challenges because the handler (route) needs to exist to initiate a
Copy link
Member

Choose a reason for hiding this comment

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

a separate RFC for async route handlers seems appropriate.

cc @dgeb / @rwjblue i think you guys have WIP or atleast ideas here.

Copy link
Author

Choose a reason for hiding this comment

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

@trentmwillis has been working on this. For what I understand, he's making getHandler function promise-aware.

Copy link
Member

Choose a reason for hiding this comment

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

That's more or less the gist of it. If you return a Promise from Router#getHandler, any Transition should wait until it resolves with a handler. I picked up the work from @dgeb and with guidance from @rwjblue. Didn't see much reason for an RFC since this change should be totally transparent to Ember/Ember-CLI consumers.

@MiguelMadero
Copy link
Author

this seems like an ember-cli rfc?

@stefanpenner I can move it there, but I thought that it was a bit broader, specially considering the integration points in router.js and ember's router.

This RFC seems to go a bit deep on brand new ideas, I think it should either focus entirely on 1 of those new ideas (like the asset-service) or it should stick to the high-level narrative between the solutions.

I'll have another pass and keep it high-level and create separate ones as needed. There's already one for addon's outputFiles and I can create another one for the AssetService.

# Unresolved questions

* Ember Inspector breaks the apps the lazy-load routes because it eagerly resolves them, which results in a default route being generated, taking precedence over the lazy loaded one. We can fix Ember Inspector and even provide information about assets related to each route once we generate the route info at compile time. However, this will require users taking a new version of the extension. Not sure if we can force an update to avoid breakign the apps or if we can avoid that behavior on the app itself.

Copy link
Author

Choose a reason for hiding this comment

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

  • Add info about LOG_RESOLVER flag to the list of unresolved questions. It seems like all the route related objects are eagerly resolved if the flag is on.

@rwjblue
Copy link
Member

rwjblue commented Nov 8, 2016

@MiguelMadero - Was this superseded by #158 and #153?

@MiguelMadero
Copy link
Author

Hi @rwjblue sorry for the late response. Yes, this was superseded by those two. There is still useful info about other use cases that are still not addressed like build multiple CSS assets, building multiple vendor-JS assets and lazy-loading from components. This was originally intended as an overview, but at this point, it probably makes more sense to close this and open others if/as needed.

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

4 participants