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

Add failing test case for rewriting the same URL multiple times #47

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

grayt0r
Copy link

@grayt0r grayt0r commented Oct 3, 2016

This PR adds a failing test case for rewriting the same URL multiple times within the same file.

Believe this demonstrates #39, as well as possibly the root cause of ember-cli/broccoli-asset-rev#99.

@rickharrison
Copy link
Collaborator

rickharrison commented Oct 9, 2016

Thanks for submitting this failing test. I've pinpointed the reason for this behind the additions in #35. The unfortunate part is I do not know how to fix this. Using a null customHash is very difficult, because we use a regex to find and loop through all the asset paths in your code base. When you change the path of the asset with a fingerprint, that particular regex will not match anymore because it is changed. However, when you don't add a hash (customHash: null), it will continue to match the number of times you have this asset. In the test, the asset appears 3 times, which is why assets/assets/assets/ is repeated 3 times.

I am beginning to think that we are trying to support too many use cases in this module. The pairing of asset-rev/rewrite is to fingerprint assets. By definition, when using a null custom hash, you are not fingerprinting the assets. I think it may be best to write a tool solely for the use case of prepending a CDN to all of your assets. Perhaps, it might help me to understand the use case. I have never shipped code in production that needs to be off a CDN, but needs to not have a hash associated with it. Why do you need to do this?

@rwjblue @stefanpenner I'd love to hear your thoughts on deprecating this particular use case and building another tool strictly for prepending to an asset and not fingerprinting.

@inkless inkless mentioned this pull request Feb 9, 2017
@typeoneerror
Copy link

@rickharrison Can't speak for others, but my use case for why this is needed is when I'm running the application locally and need to prepend "http://localhost:4200/" because I'm loading the ember template in a Rails application and serving at different subdomains (sub1.server.dev:3000, sub2.server.dev:3000, for example). In this case, I don't want fingerprinting because it takes 20-30 seconds on each build. With customHash=null, I get the prepend feature (builds in about 5-8 seconds instead!) but the URLS end up as /assets/assets/assets. I agree that this use case is pretty edge, but it does seem like this is a recurring need for multiple ember devs. Happy to hear about a different solution that avoids this!

@typeoneerror
Copy link

FYI, this is still an issue, but I found out today (after stumbling into my original bug report in 2016) that if you pass literally anything else to customHash but null, it works. So I just did:

customHash: 'file'

Now you effectively have a fixed file name but prepend still works.

I don't expect this bug to ever be fixed since it's still around, but this is a good workaround as this is a bit of an edge case (though not that much of an edge case seeing that this is the third project I've run into it on ;))

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.

3 participants