Open fedegl opened 4 years ago
Thanks for the report.
From the error it looks like you are linking to a .eot
font which doesn't exist. Is this a correct interpretation, or does that .eot
font actually exist in your file system?
If the repository for this page is public, could you link to it to help me recreate the problem?
@ehmicky No stack traces in the netlify-build output any more?
There should be a stack trace. I am not sure why this is not shown in this specific case though.
@Munter The file is there.
The repository is not public, but I could grant you access if that would be useful.
@Munter Ha, I bumped into this issue too and was trying to work up a patch just now :slightly_smiling_face:
Some debugging revealed that graph.findAssets()
is returning the same file multiple times, at least in my project. This means that the result
array gets multiple entries with the same { from, to }
pair. Thus, the problem here is that we're trying to delete the same path multiple times. The second deletion obviously fails.
Here's the patch I've applied:
diff --git a/plugins/netlify-plugin-hashfiles/lib/index.js b/plugins/netlify-plugin-hashfiles/lib/index.js
index bdc8a3f..1ea7f36 100644
--- a/plugins/netlify-plugin-hashfiles/lib/index.js
+++ b/plugins/netlify-plugin-hashfiles/lib/index.js
@@ -38,21 +38,20 @@ module.exports = {
await hashfiles(graph, { trimmedStaticDir });
- const result = [];
+ const result = new Map();
for (const asset of graph.findAssets({ isLoaded: true, isInline: false })) {
const from = pathMap.get(asset);
const to = asset.url.replace('file://', '').split('?')[0];
if (from !== to) {
- result.push({
- from,
- to,
- });
+ result.set(from, to);
}
}
- const deletePromises = Promise.all(result.map((r) => deleteFile(r.from)));
+ const deletePromises = Promise.all(
+ Array.from(result, ([from, to]) => deleteFile(from))
+ );
const writePromise = await graph.writeAssetsToDisc(
{ isLoaded: true },
@@ -75,11 +74,10 @@ module.exports = {
console.log('** hashfiles moved files: **');
console.log(
- result
- .map(
- ({ from, to }) => `${relative(root, from)} ==> ${relative(root, to)}`
- )
- .join('\n')
+ Array.from(
+ result,
+ ([from, to]) => `${relative(root, from)} ==> ${relative(root, to)}`
+ ).join('\n')
);
},
};
I guess one advantage of this is that it also fixes the console output -- with your fix above, the output will still show the same path repeatedly.
FWIW, I find it surprising that this plugin deletes the original files at all... it doesn't really seem necessary?
@jonleighton Lol, this is great timing! I couldn't figure out how to recreate the problem, so I just opted for the lazy solution to avoid people being blocked.
I'm a bit surprised you're finding that assetgraph returns the same asset multiple times in that query. This sounds like a bug in assetgraph, since each asset should be deduplicated using the URL as an identifier during graph population.
Do you have a way to recreate this problem? I'd love if you could submit a PR for a test case. I just added a bunch in the test folder, so it should be a bit easier to help recreate bugs like these now
FWIW, I find it surprising that this plugin deletes the original files at all... it doesn't really seem necessary?
It isn't strictly necessary. It's an attempt at reducing the distribution size so the deployment to the CDN doesn't become slower
Sure, I'll try to dig into it a bit more to figure out why it's returning the same asset multiple times :+1:
It isn't strictly necessary. It's an attempt at reducing the distribution size so the deployment to the CDN doesn't become slower
Would be nice to provide a way to turn this off. I just came across a problem where I have:
<meta name="og:image" content="https://my.site/path/to/photo.jpg">
It isn't getting rewritten by the plugin (possibly due to the absolute URL?), so is now a broken link. However, AFAICT an absolute URL is mandatory for this.
This sounds like a separate bug. Assetgraph does support open-graph images and it also does have the capability to classify an absolute URL as an internal URL, provided that the graphs canonicalRoot
is set and matches with the absolute URL. If that content
attribute is not updated to point tp the hashed asset there is either a misconfiguration of the canonicalRoot or a bug in Assetgraph
I think I might be coming around to not deleting the original files though. There might very well be cases of source references that assetgraph hasn't yet implemented. And if you have an asset that is references by a combination of relations that are supported and ones that are not supported, only the supported ones will get rewritten, and the unsupported ones will indeed link to the now deleted original asset.
I'll remove the file deletion code.
I'd still like to track down both the double asset listing and this open-graph image case
Sounds good :+1: I agree there are likely to always be edge cases where the deletion of the original image might be confusing.
That said, I think the og:image
problem was my fault. The meta tag should be:
<meta property="og:image" ...>
But I was using:
<meta name="og:image" ...>
When I switched to the correct markup, assetgraph correctly found and replaced the image URL.
I'll try to dig into the duplicate paths problem more tomorrow.
Test case submitted here. Though I'm not sure how similar my problem is to @fedegl's, since mine relates to the use of srcset
with the same image path repeated multiple times with a different query string, but the original issue related to a font file.