Use HTML meta to store asset map information #40
Conversation
|
O, @krallin thank you very much :) We had a big discussion in this one - #37. I've also got to a conclusion, that putting it into vendor.js wasn't right. When I've tried to fix it properly, I've quite got into a spaghetti of what comes before what and it kinda delayed my implementation. I'll check yours, but can you please check tests? Thank you :) |
There are a couple downsides to rewriting vendor.js in-place, which are documented in adopted-ember-addons#36: - It breaks SRI - It doesn't update the fingerprint for vendor.js, even though the contents have changed. This patch fixes the issue by loading the asset map information (either a JSON object or a URL for the asset map) into a HTML meta tag that is stored in `index.html` instead, then read at runtime by `getAssetMapData` (the meta tag is formatted as JSON and URL-encoded). Conceptually, this is fairly similar to the `storeConfigInMeta` option for the Ember build that stores the app's environment in a meta tag (https://ember-cli.com/user-guide/#application-configuration).
|
@RuslanZavacky yep, just fixed tests (the root cause there was that we weren't interpolating |
|
Hey, my app recently ran into an issue in prod dealing with what our team expects is due to the vendor.js file's hash not changing and referencing an old assetMap hash. This PR looks like a step in the right direction, and IMO better tradeoff than rewriting vendor.js at build time. Also, falls in line with ember-cli storing config in meta tags. Is there anything I can help out with in getting this merged in? |
addon/utils/get-asset-map-data.js
Outdated
| export default function getAssetMapData() { | ||
| // This placeholder is replaced in the build step | ||
| return '__asset_map_placeholder__'; | ||
| const assetMapString = $("meta[name='ember-cli-ifa:assetMap']").attr('content'); |
There was a problem hiding this comment.
In the interest of supporting this RFC, it might make more sense to use
document.querySelector("meta[name='ember-cli-ifa:assetMap']").content and not use jQuery in this file.
There was a problem hiding this comment.
Good call; thanks. Updated my PR.
|
@jbailey4 I think this is in @RuslanZavacky's hands at this time: @RuslanZavacky, let me know if you have any thoughts on this PR 😄 |
addon/utils/get-asset-map-data.js
Outdated
| export default function getAssetMapData() { | ||
| const assetMapString = $("meta[name='ember-cli-ifa:assetMap']").attr('content'); | ||
| const assetMapString = document.querySelector("meta[name='ember-cli-ifa:assetMap']").content; |
There was a problem hiding this comment.
Tried to run ember s with this change, it now complains on ember fastboot level, that document is not defined. For fastboot I think we have to come up with something different...
There was a problem hiding this comment.
That's super disappointing...if we have to stick with jQuery to support fastboot, then so be it, I guess. :\
There was a problem hiding this comment.
Or just guard against it, as suggested here.
|
So I've just spent a few hours today trying to figure out why SRI was failing on my build, and it turns out it was this addon (for the above mentioned reasons - vendor.js being modified but new hash not generated). I'm not sure why it's only just started to fail on me (I did just recently update to ember 3.2 and updated a bunch of packages). I'm running 0.7.0 ember-cli-ifa, and 2.1.1 ember-cli-sri I tried setting Directly referencing this PR though, fixes my SRI issues (in testing/staging env). However I encounter an error when building for production. The error looks like it is because it can't find the file tests/index.html in the srihash_assets-output_path. If I comment out this line in the PR, it builds correctly. Here's the error log |
|
A more permanent fix, works for both environments: let filePath = path.join(build.directory, 'index.html');
replacePlaceholder(filePath, assetMap);
filePath = path.join(build.directory, 'tests/index.html'); // May not exist, eg prod build
if(fs.existsSync(filePath)) {
replacePlaceholder(filePath, assetMap);
} |
|
I'd really like if we can get this PR across the line and merged/published |
|
Is there anything else we need for this? @krallin @RuslanZavacky I'm not sure how to add my above fix to this PR, but I'm happy to help test once its there. |
|
Hi @krallin have you tried your PR with Fastboot? If I run Thanks — it would be great to get this working because the current master branch doesn't seem to inline the asset map data into |
|
@chrism I don't remember - considering this PR has been stale for a few months at this point, I'm not really investing time in this anymore, so I'm afraid I can't really help you :/ |
|
@krallin completely understand - thanks for getting back to me. Just out of interest (hope you don't mind me asking) but have you found an alternative approach to solve this problem of fingerprinting? |
|
@chrism I'm just using this patch from my branch. We don't use fastboot, though, which is why I can't really help you with that bit; I'm sorry! |
|
@krallin you've been a big help—i'm just trying to gauge whether to spend time trying to get it to work with Fastboot or not. Thanks again! |
|
Sorry guys, I am not moving forward with it, as I wasn't still convinced that thats the best way to go.. as far as I see, the best way is to revert as it was pre-change to move code to vendor.js and upgrade to latest ember. Thats still seems that the only way, that worked well for fastboot and non-fastboot modes. |
|
Actually, disregard my comment. Wrongly assumed something different. This PR looks good, but need to check more into fastboot land. |
|
Thank you @krallin. I will test it out in master and we'll see if something needs some improvements. |
|
Thanks for the info. I'm using (almost) latest Ember 3.4.5 — but as I was unable to get this addon working inline by just installing with This PR works for me (with Fastboot disabled), but I'd like the continue using Fastboot Personally I like the approach of using a meta tag, but I don't really have any strong opinions I'd just like to be able to access the fingerprinted asset URL from within a component. If you have any advice for a currently stable version working with Fastboot and Ember 2.4.5 I'd go with that. Thanks for the hard work on this. |
|
I've merged this PR but remembered, why I've hesitated. For Fastboot to work with this PR, inline mode should work via global variable. It doesn't seem that there are any other viable options. I think I'll make a inline mode to be the same JSON object as it was before, as I am out of options how to make it work with meta for inline with fastboot. If anyone has better ideas, I'd be more than welcome :) |
|
@RuslanZavacky thanks very much for looking into this it looks like your PR #44 might address the error message I was getting with Fastboot too. I hesitate to offer any advice about how it could work with Fastboot as I'd need to really dig into the code, but could using the shoebox be a good option? It already exists and seems to be a place dedicated to storing data in this way? https://github.com/ember-fastboot/ember-cli-fastboot#the-shoebox. Just out of interest—do you plan on working on a version which also supports Fastboot as it was before, or awaiting a PR from someone with an implementation? |
|
Hi again @RuslanZavacky I've been looking into alternatives to a global variable for the Fastboot code path and think I have a possible solution. Before I make a PR for an implementation I want to run it by you so I am not wasting my time on this. There is a working version here: https://github.com/chrism/test-fastboot-config-from-addon to illustrate the concept. Basically, instead of a global variable I'm taking advantage of the fastbootConfigTree() {
return {
[this.app.name]: {
"asset-map": {
"foo": "bar",
"something": "different"
}
}
}
}And using the <meta name="asset-map" content="%7B%22asset-map%22%3A%7B%22foo%22%3A%22bar%22%2C%22something%22%3A%22different%22%7D%7D">With these two options it means that instead of an issue with In the test app there is a component with this code: import Component from '@ember/component';
import { inject as service } from '@ember/service';
export default Component.extend({
fastboot: service(),
init() {
this._super(...arguments);
let configHash;
if (this.get('fastboot.isFastBoot')) {
configHash = FastBoot.config().default;
} else {
let rawConfig = document.querySelector("meta[name='asset-map']").content;
configHash = JSON.parse(decodeURIComponent(rawConfig));
}
this.config = configHash['asset-map'].foo;
}
});Which seems to work well, for both Fastboot and javascript environments. There is still some work to investigate whether this will be easily applied to the ifa build process, but i'm confident that it should be OK. My question to you is—should I spend time creating a PR with this approach for you, would you merge it? Or should I consider creating a new addon completely. My personal preference would be to contribute to this project but I don't know your time constraints/commitment to it and completely understand if it isn't a priority for you. Thanks. |
|
Glad this got in! @RuslanZavacky Any chance this will be released in the next version? |
There are a couple downsides to rewriting vendor.js in-place, which
are documented in #36:
contents have changed.
This patch fixes the issue by loading the asset map information (either
a JSON object or a URL for the asset map) into a HTML meta tag that is
stored in
index.htmlinstead, then read at runtime bygetAssetMapData(the meta tag is formatted as JSON and URL-encoded).Conceptually, this is fairly similar to the
storeConfigInMetaoptionfor the Ember build that stores the app's environment in a meta tag
(https://ember-cli.com/user-guide/#application-configuration).