Skip to content

Conversation

@ferlores
Copy link
Contributor

@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

index.js Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it!

@dgbeck
Copy link
Member

dgbeck commented Oct 26, 2015

Looks great in general @ferlores!

@ferlores
Copy link
Contributor Author

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

@ferlores
Copy link
Contributor Author

@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
Copy link
Member

dgbeck commented Oct 28, 2015

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
Copy link
Contributor Author

@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
Copy link
Member

dgbeck commented Oct 28, 2015

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 [email protected]
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
#45 (comment)
.

@ferlores
Copy link
Contributor Author

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dgbeck that's how I fixed the test

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

@ferlores
Copy link
Contributor Author

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

dgbeck added a commit that referenced this pull request Oct 30, 2015
Add support for cache busting assets - sync version
@dgbeck dgbeck merged commit 914900e into rotundasoftware:master Oct 30, 2015
@ferlores
Copy link
Contributor Author

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

@dgbeck
Copy link
Member

dgbeck commented Oct 30, 2015

Published to v5.3.0!

@ferlores ferlores deleted the assets branch October 30, 2015 23:54
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

Successfully merging this pull request may close these issues.

2 participants