Handle fingerprinted asset maps #262
Conversation
| @@ -104,7 +104,9 @@ FastBootConfig.prototype.readAssetManifest = function() { | |||
| var assetMap = JSON.parse(fs.readFileSync(assetMapPath)); | |||
| return assetMap; | |||
| } catch (e) { | |||
| // No asset map was found, proceed as usual | |||
| if (assetMapPath) { | |||
There was a problem hiding this comment.
We should at least warn about this when you have an asset map.
There was a problem hiding this comment.
The warning should only show if the assets are fingerprinted and there is not asset map, right? That would be a bad state. If they aren't fingerprinted, then they would be at their expected paths, and the asset map isn't required (As least that's my understanding)
There was a problem hiding this comment.
I thought about that. Any time getting an assetMap load fails we should log it. If it's a malformed assetMap that fails parse or if it's just absent, the user would benefit from knowing that. We can print the error too.
There was a problem hiding this comment.
I think it may be beneficial to include the error message and also state that it's warning, rather than an error in the case that it's not required (non-fingerprinted assets)
There was a problem hiding this comment.
IMO, you should only emit warnings if they are actionable. In this case, it will always emit a warning in development mode that you can't get rid of, because asset rewriting is not turned on in development mode. That would drive me crazy, and it will make people less likely to see other, real warnings that we might emit in the future.
There was a problem hiding this comment.
In other words, I'm in favor of emitting a warning or error if we can't parse the file, or if we would expect the file to exist and it doesn't, but I am not in favor of logging a warning always in development mode if fingerprinting is turned off (which it is by default).
There was a problem hiding this comment.
This warning (especially after taking care of @danmcclain's feedback) will only trigger if asset map generation is enabled and no asset map was found. I think that is absolutely a scenario that warrants a warning.
There was a problem hiding this comment.
Per the discussion at this week's meeting:
This warning should only be emitted if assetRev && assetRev.options && assetRev.options.enabled.
arjansingh
force-pushed
the
assetmap
branch
2 times, most recently
from
cfa1558
to
84bcf6a
Compare
| var assetMapPath = path.join(this.inputPaths[0], 'fastbootAssetMap.json'); | ||
|
|
||
| try { | ||
| var assetMap = JSON.parse(fs.readFileSync(assetMapPath)); | ||
| return assetMap; | ||
| } catch (e) { | ||
| // No asset map was found, proceed as usual | ||
| if (assetMapPath) { | ||
| ui.writeLine(fmt("FastBoot Build: No asset map was found at: %s", assetMapPath), ui.WARNING); |
There was a problem hiding this comment.
@danmcclain switched to using this.ui as well.
|
Good to merge? |
|
Any luck reviewing @danmcclain @tomdale? |
arjansingh
force-pushed
the
assetmap
branch
2 times, most recently
from
3be6fa5
to
edcd3af
Compare
| @@ -98,13 +98,16 @@ FastBootConfig.prototype.buildDependencies = function() { | |||
| }; | |||
|
|
|||
| FastBootConfig.prototype.readAssetManifest = function() { | |||
| if (!this.assetMapEnabled) return ''; | |||
There was a problem hiding this comment.
Since we know this before the try/catch block I decided to leverage it to do an early exit. Let me know if you'd rather I didn't.
There was a problem hiding this comment.
Nevermind this comment.
arjansingh
force-pushed
the
assetmap
branch
from
edcd3af
to
4344b6b
Compare
…ox-after-request Build new sandbox after request is done
broccoli-asset-rev has the option to fingerprint your asset map file.
This PR adds support for that scenario.
I'm not happy about using the wild card, but I don't think a
RegExpwill work there.