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

RFC: Asset Manifest #153

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@trentmwillis
Member

trentmwillis commented Jul 17, 2016

Rendered.

Additional context:

@nathanhammond nathanhammond referenced this pull request Jul 17, 2016

Closed

Lazy Loading Engines Attack Plan! #154

18 of 29 tasks complete
@dgeb

This comment has been minimized.

Member

dgeb commented Jul 18, 2016

Since we are only concerned with the subset of URIs that are URLs, we should consider using url or perhaps src instead of uri to specify the location of each bundle.

@trentmwillis

This comment has been minimized.

Member

trentmwillis commented Jul 18, 2016

@dgeb we could potentially switch to url, the primary reason I went with uri is that depending on the nature of your resource delivery, you could just have an ID or some path that is not necessarily a resolved URL.

I'd also like to avoid src as it feels specific to JS assets.

@dgeb

This comment has been minimized.

Member

dgeb commented Jul 18, 2016

depending on the nature of your resource delivery, you could just have an ID or some path that is not necessarily a resolved URL.

Good point - let's stick with uri 👍

@nathanhammond

This comment has been minimized.

nathanhammond commented Jul 19, 2016

Capturing a note from a Slack conversation I believe originally brought up by @dgeb:
The asset manifest in this case would be a product generated at the very tail end of the buildprocess (sometimes even after deploy-specific post-processing) which will then need to be inserted into index.html which will already have a serialized config meta tag.

This isn't particularly elegant and is something we should think more about.

@nathanhammond

This comment has been minimized.

nathanhammond commented Jul 21, 2016

Note from @rwjblue: should address the fact that the assetmanifest.json is not a complete enumeration of all engine assets, it excludes:

  • Fill...
  • ...in...
  • ...list.
@trentmwillis

This comment has been minimized.

Member

trentmwillis commented Jul 22, 2016

Updated. Changes discuss generating an actual file (asset-manifest.json), handling of on-demand static assets (e.g., anything in public), and overrides/customization.

@trentmwillis trentmwillis changed the title from Asset Manifest to RFC: Asset Manifest Jul 22, 2016

@nathanhammond

This comment has been minimized.

nathanhammond commented Jul 22, 2016

Changes look good. Do we want to mention asset-rev rewriting of the asset-manifest.json in this or leave that as unspecified behavior per this RFC?

@trentmwillis

This comment has been minimized.

Member

trentmwillis commented Jul 22, 2016

@nathanhammond it mentions accommodating asset-rev twice, thought doesn't go into detail on what needs to be done as I feel that is outside the scope of this RFC

@rwjblue

This comment has been minimized.

Member

rwjblue commented Nov 8, 2016

Where are we at with this? Discussion seems to be mostly quieted down.

@dgeb / @MiguelMadero / @nathanhammond / @trentmwillis - Are more changes needed here or are we ready to try to advance to FCP?

@rwjblue rwjblue referenced this pull request Nov 8, 2016

Closed

Adds lazy-load-overview #141

1 of 4 tasks complete
@trentmwillis

This comment has been minimized.

Member

trentmwillis commented Nov 8, 2016

We have a working implementation in the ember-asset-loader addon. Some changes have occurred in how we generate the manifest, so I'll need to go and see if that requires any updates to the RFC as it stands here. Will try to do that by end of week.

Each bundle will have two primary fields: `assets` and `dependencies`.
`assets` is an array of asset objects, which specify a `uri` and `type` for an asset to be loaded. The asset objects are sorted according to load order. This is important for assets such as `vendor.js` which need to be loaded before other assets like `app.js`.

This comment has been minimized.

@MiguelMadero

MiguelMadero Nov 28, 2016

The asset objects are sorted according to load order

I didn't see anything about order on the AssetLoader RFC. We had problems with this before. Not all browsers handle this the same and we had to explicitly set async=false to make sure that the scripts are really executed in the order they are added.

I think we need to do it on https://github.com/trentmwillis/ember-asset-loader/blob/master/addon/loaders/js.js#L20

This comment has been minimized.

@MiguelMadero

MiguelMadero Nov 28, 2016

It's easier if I create a PR to follow-up the discussion there.

This comment has been minimized.

This comment has been minimized.

@trentmwillis

trentmwillis Dec 3, 2016

Member

I added some additional language about ordering and the relationship between the manifest ordering and behavior of the loader.

@MiguelMadero

This comment has been minimized.

MiguelMadero commented Nov 28, 2016

Hi @trentmwillis and all, I've been quietly following this along. I didn't have much to add and mostly agreed with this. It's really similar to what we used already with some minor improvements. I'm in general pretty happy with this as it's.

I'll need to go and see if that requires any updates

Does this still require an update? It looks ready for FCP.

@MiguelMadero

Minor comment about load order

@trentmwillis

This comment has been minimized.

Member

trentmwillis commented Dec 3, 2016

@rwjblue I've updated with some clarifications. At this point, I think we're ready to proceed to FCP if no objections are raised.

@rwjblue

This comment has been minimized.

Member

rwjblue commented Apr 28, 2017

We discussed this during todays Ember core team meeting today, and we are 👍 on this moving forward.

Given that there is little conversation here, and the core team is in favor I an moving this forward into the final comment period.

@tomdale

This comment has been minimized.

Member

tomdale commented May 1, 2017

I was at the core team meeting but I had some questions post-meeting. 😁

  1. It seems like there is some overlap between this and dynamic imports/import().then(). Is it possible to align this proposal with that proposal, instead of creating Ember-proprietary APIs?
  2. I suspect Webpack and others are generating similar manifests (e.g. to support dynamic imports). Is this something we can align with?
  3. FastBoot also generates a manifest file that the FastBoot server uses to know which files to load. Does it make sense to move that manifest out of something FastBoot-specific and into this?
@blittle

This comment has been minimized.

blittle commented May 4, 2017

@joeldenning and I built a similar tool on top of System.js called sofe. We have really liked using the new dynamic imports syntax. Utilizing System.js's module registry makes sharing common dependencies easy as well. As far as webpack, we've had multiple discussions with @TheLarkInn on Twitter about how webpack doesn't have plans to do this type of dynamic loading.

One major suggestion I'd have for your implementation is to build local overrides. For example, in sofe I could load a module:

import('my-app!sofe').then(...)

If there is a local storage override:

localStorage.setItem('my-app:sofe', 'https://localhost:3333/my-app.js')

Then in the context of production, I can override the asset manifest location of a specific bundle to point to my localhost. This makes testing and development in production really easy. I can override the specific bundle that I care about in the exact context of prod!

@rwjblue

This comment has been minimized.

Member

rwjblue commented Mar 9, 2018

Discussed this a bit with @tomdale and we decided that we should close this. Not as "wontfix" but more as "already done" in addon space without needing more detailed core framework involvement. This is absolutely public API from ember-engines point of view, and it will be supported as such.

@trentmwillis - Thank you very much for your hard work on this, and I'm very sorry that we let this sit for sooo long.

@rwjblue rwjblue closed this Mar 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment