rotundasoftware / cartero

Modular front end development for multi-page web applications.
MIT License
204 stars 25 forks source link

Add support for cache busting assets - sync version #45

Closed ferlores closed 8 years ago

ferlores commented 9 years ago

@dgbeck Here is the PR where I calculate the shasum in a sync way so it doesn't need changes in parcelify. I think it makes sense so far. Please let me know what you think.

Related to #43

dgbeck commented 9 years ago

Looks great in general @ferlores!

ferlores commented 9 years ago

Great @dgbeck I'll rework your comments and let you know when is ready

ferlores commented 9 years ago

@dgbeck I'm also looking into travis: https://travis-ci.org/rotundasoftware/cartero/builds/87563089 Somehow it fails the tests becasue in travis it generates bundles with another shasums. Locally it works well for me. Can you please pull the branch and check if it works locally for you? Thanks!

dgbeck commented 9 years ago

Hi @ferlores . The issue here is that the contents of the bundles, for example, the css bundle, contain the package id 1a85dd866e0688cef5788a8e0ceb5a2b6d3fb0a2:

body {
    background : blue url( '/1a85dd866e0688cef5788a8e0ceb5a2b6d3fb0a2/img/robot_9018f21e83ce46f3ea2e3b73e5d75ece75407df7.png' );
}

The package id is a shasum that is calculated in part from the the full path of the package directory. See getPkgId in parcel-map. The full path changes from system to system.

Two solutions come to mind. The first is just to not test the name of those bundles. The second is to pass the cartero option appRootDir all the way through to parcel-map so that the package ids can be kept consistent from system to system as long as appRootDir is supplied.

In the interest of just getting this PR merged as quickly as possible, I'm fine with just not testing the names of the bundles and we can revisit other options later.

ferlores commented 9 years ago

@dgbeck Thanks for the explanation. It kind of makes sense except for the part that in the other tests you are testing for the bundles names (https://github.com/rotundasoftware/cartero/blob/master/test/test.js#L37) and that doesn't fail. Any idea what I'm doing different here that it doesn't work?

I'm fine with no testing that but it just gives me a bad feeling not really understanding why it doesnt work in this case. I dont want to introduce bugs here Thanks!

dgbeck commented 9 years ago

Hi Fernando,

The bundle names themselves are not the issue... It is the package id in the bundle contents that is causing the names to change.

On Wednesday, October 28, 2015, Fernando Lores notifications@github.com wrote:

@dgbeck https://github.com/dgbeck Thanks for the explanation. It kind of makes sense except for the part that in the other tests you are testing for the bundles names ( https://github.com/rotundasoftware/cartero/blob/master/test/test.js#L37) and that doesn't fail. Any idea what I'm doing different here that it doesn't work?

I'm fine with no testing that but it just gives me a bad feeling not really understanding why it doesnt work in this case. I dont want to introduce bugs here Thanks!

— Reply to this email directly or view it on GitHub https://github.com/rotundasoftware/cartero/pull/45#issuecomment-151968359 .

ferlores commented 8 years ago

@dgbeck Ok, finally I've got the issue. The problem is that the shasum for js files is calculated after we do the first pass replacing the content of ##asset_url(./relative/path) with the absolute path ##asset_url(/absolute/path). Therefore the content of the file is different depending on where you run cartero, leading to have different shasums if (and only if) you are using ##asset_url.

For now I will comment out that test since I feel it is out of the scope of this change

ferlores commented 8 years ago

@dgbeck the PR is ready to merge with tests passing, please let me know what you think. Thanks!

ferlores commented 8 years ago

@dgbeck thanks for the merge! let me know when you have a chance to publish it

dgbeck commented 8 years ago

Published to v5.3.0!