Skip to content
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

Question on fingerprint file name for bundled images #40

Open
leslc opened this issue Sep 3, 2015 · 9 comments
Open

Question on fingerprint file name for bundled images #40

leslc opened this issue Sep 3, 2015 · 9 comments

Comments

@leslc
Copy link

leslc commented Sep 3, 2015

(edited after more research)

Hi @dgbeck,

I'm seeing an issue where a different image has the same fingerprint in the filename. How is the fingerprint figured out for images? Is it based on size? This might be an edge case where they are the exact same size but with different content.

Thanks

@leslc leslc changed the title Asset directory names are the same after each build Question on fingerprint file name for bundled images Sep 3, 2015
@leslc
Copy link
Author

leslc commented Sep 4, 2015

I tested this out a bit more. Changing the image (keeping the same file name) does not change the directory fingerprint name. Is it possible to fingerprint by file size of images? If not, can we add a random suffix to the end of the directory name every time?

@dgbeck
Copy link
Member

dgbeck commented Sep 4, 2015

Hi @leslc !

First thought on this matter is that the directory names (which are in actuality package ids) would be difficult to change on every pass. Especially in watch mode it is very convenient for them to be stable.

Taking a step back, what particular issue are you running into because of the images maintaining their same name / path? Is it a caching issue?

@ferlores
Copy link
Contributor

ferlores commented Sep 7, 2015

Hi @dgbeck,
The problem that we are having with @leslc is that the images don't have a distinct url when the content of them changes. CDNs have caching problems because of that. Have you had problems with this before? I believe cartero should treat images in the same way it does with js and css files.

Thanks
F

@dgbeck
Copy link
Member

dgbeck commented Sep 10, 2015

Hi @ferlores!

Interesting. We have not run into this issue before but I can see why what you are suggesting could be helpful.

If we were to go down this path, images urls / names would need to change in two places

  • in css files that reference those images. For this case, we are already doing a relative to absolute url transform to all style assets, so it doesn't seem too crazy to add some extra logic for the shasum.
  • in any html files (or other assets) that reference those images. In this case, either ##asset_url() or CarteroNodeHook.prototype.getAssetUrl needs to be used anyway in order to find the image url, so we could add some extra logic in there for the shasum. (We'd need to add some more information to metaData.json file so we could map the old image urls to the new ones at runtime.)

However I'm hesitant to build this into cartero core since it seems very closely coupled to CDNs, and adds some significant complexity.

Also of note, is that currently we are only adding shasums to bundle names, but if we start adding them to images, for consistency we should probably add them to individual css files, and potentially other (custom) asset types.

Talking another a step back, which CDN are you using? Why can you not just invalidate / re-upload the images when you do a new deploy?

@leslc
Copy link
Author

leslc commented Sep 10, 2015

Taking CDN out of the conversation for now, we'd need to fingerprint the image file names and/or directory for the exact same reasons fingerprinting is used for JS and CSS files -- utilize the browser cache if no image changes have occurred and always load an image that has changed.

It would be best to change the fingerprint of the file name/directory only when the image changes. But it would be acceptable to change the name upon every build of images. It won't be the best for utilizing the browser cache, but it's better than the browser showing the old image from the cache.

@leslc
Copy link
Author

leslc commented Sep 10, 2015

Thanks for pointing out the locations of the changes. We'll take a look and submit a PR.

Would you be more comfortable with a configuration that controls the image file name/directory fingerprinting? (I personally think it should be the default to prevent browser and CDN cache issues).

@dgbeck
Copy link
Member

dgbeck commented Sep 11, 2015

Ok. It will be an interesting exercise, if nothing else!

My preference is to introduce an option, say assetTypesToCacheBust, that expects an array of asset types that should have the shasumed of their contents appended to their name, similar to the assetTypesToConcatenate option. We can default to [ 'script', 'style' ], since that what we are doing already (except that we only currently do this for bundled assets, whereas this option would also add the shasum to individual style assets).

Note that this will not be an easy PR. Couple considerations...

It seems like we might want to bake some of this new logic directly into parcelify's Asset class. Right now we have a bit of an awkward line in cartero that changes the extension on all style assets to .css. There is a possibility of giving the Asset class the ability to determine a new file name for itself when it is written to disk, and kill both these birds with that stone (figuratively, not actually killing birds).

Also we will need to store the mapping of old asset names to new names somehow in the metaData.json file, so that the url of an asset can be determined at runtime given its source path with CarteroNodeHook.prototype.getAssetUrl(). Maybe we can do that as phase 2 of this PR at just keep in mind that we'll need that functionality for now.

Where are you guys located?

@ferlores
Copy link
Contributor

Hi @dgbeck
Thanks for being open to the exercise of adding this feature. We are located in downtown San Francisco, maybe we can meetup somewhere to discuss some ideas before jumping into coding.
Thanks
F

@dgbeck
Copy link
Member

dgbeck commented Sep 16, 2015

Hi @ferlores , nice. We are in SOMA and yeah seems like it would be helpful to bounce around some ideas in person. I will email you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants