-
-
Notifications
You must be signed in to change notification settings - Fork 408
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
RFC: Asset Manifest #153
Conversation
Since we are only concerned with the subset of URIs that are URLs, we should consider using |
@dgeb we could potentially switch to I'd also like to avoid |
Good point - let's stick with |
Capturing a note from a Slack conversation I believe originally brought up by @dgeb: This isn't particularly elegant and is something we should think more about. |
Note from @rwjblue: should address the fact that the
|
Updated. Changes discuss generating an actual file ( |
Changes look good. Do we want to mention asset-rev rewriting of the |
@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 |
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? |
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`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's easier if I create a PR to follow-up the discussion there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some additional language about ordering and the relationship between the manifest ordering and behavior of the loader.
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.
Does this still require an update? It looks ready for FCP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment about load order
1e27fa6
to
74b89e9
Compare
@rwjblue I've updated with some clarifications. At this point, I think we're ready to proceed to FCP if no objections are raised. |
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. |
I was at the core team meeting but I had some questions post-meeting. 😁
|
@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! |
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. |
Rendered.
Additional context: