WIP: Content-hash bundle names #829
Conversation
src/utils/packageHasher.js
Outdated
| walkBundles(child, indent + '--') | ||
| } | ||
| } | ||
| hash = md5File.sync(bundle.name); |
There was a problem hiding this comment.
Above this line, we need to do a find and replace on the current file (bundle.name) with the contents of walkedFiles (which has a map from old hashes to new hashes).
benhutton
force-pushed
the
feature/cache-busting
branch
2 times, most recently
from
56793a6
to
5fed71c
Compare
src/utils/packageHasher.js
Outdated
| data = fs.readFileSync(fileName, 'utf8') | ||
|
|
||
| hashMap.forEach(function(value, key) { | ||
| data = data.replace(new RegExp(key, 'g'), value); |
There was a problem hiding this comment.
This seems somewhat dangerous and would probably end up replacing things we don't want. For example, if you had a file called index.js but some string in the app also contained that but wasn't a reference, we'd end up replacing it too.
Instead of doing it this way, maybe we can store a list of reference locations as part of the asset (e.g. part of the options to addDependency)?
There was a problem hiding this comment.
@devongovett in my limited experience, bundle.name has only ever been my entry file (entry.html in my case) or an md5 hash. What are the other possibilities?
There was a problem hiding this comment.
For example, if you use the __filename variable in your code, it will be replaced with the current input filename (e.g. index.js). We wouldn't want that to be replaced with the generated bundle name.
There was a problem hiding this comment.
@devongovett in my testing, hashMap only contains md5 hashes of things. Are there other non-md5 names that can get assigned to a bundle?
There was a problem hiding this comment.
See logic here for how bundles are named: https:/parcel-bundler/parcel/blob/master/src/Asset.js#L178-L213
There was a problem hiding this comment.
@devongovett okay....
If a name isn't hashed in generateBundleName(), would we want this new code to hash it? What's the reason for leaving it as-is there and not just hashing everything?
There was a problem hiding this comment.
My point is mostly that doing a search and replace over output files for filenames is somewhat dangerous in that we might end up replacing things we didn't intend. Perhaps generateBundleName could generate something random always (rather than nice looking filenames like index.js) which is much less likely to appear in the source code, and we could use that to replace with the final names here.
There was a problem hiding this comment.
@devongovett yeah, if it's safe for generateBundleName to always use hashing, that would be nice.
benhutton
force-pushed
the
feature/cache-busting
branch
from
5fed71c
to
9038b02
Compare
| @@ -200,7 +200,7 @@ class Asset { | |||
| const packageName = sanitizeFilename(this.package.name, { | |||
| replacement: '-' | |||
| }); | |||
| return packageName + ext; | |||
| return md5(packageName) + ext; | |||
There was a problem hiding this comment.
@devongovett is all I need to do in this function? My intent is to not do any renaming on the entry file: https:/parcel-bundler/parcel/pull/829/files#diff-784c5f1cd4c785db2834e618a52b438eR35
|
Closing in favor of #1025. |
Still a long way to go to get this ready for review. And the javascript you see here is that of a Rubyist stumbling into javascript and its asynchronicity. We'll get this cleaned up in the next few days and try to write a few tests and submit a proper PR template.
Fixes #717
Fixes #188
/cc @shanebo