Fix prepend option in srcset attributes #61
Conversation
The problem in ember-cli/broccoli-asset-rev#48 happened because the logic to avoid prepending assets twice did not take into account the edge case of srcset, where there are many assets in one match. This edge case complicates things more than a bit. To simplify the solution, when a prepend option is found, we use a replacer function as second parameter. Closes ember-cli/broccoli-asset-rev#48
| /* | ||
| * Checks if there is already a prepend in the current match. | ||
| */ | ||
| function alreadyHasPrepend(string, prepend, offset, submatch) { |
There was a problem hiding this comment.
perhaps use function alreadyHasPrepend(string, prepend, offset, submatch = '') { to avoid ugly (submatch || "").length
|
Seems reasonable to me, but I'm unsure how to gauge the potential side effects... |
|
On that regard, I think we could merge this and also bump the minimum node version, and then use that as an excuse for bumping the mayor version. |
|
@rwjblue ping on this. |
|
@rwjblue dockyard.com is using this fix in production without any problem. Can this land? |
|
To be more specific, we used a fork of
We haven't seen any issue with it and are planning on using |
|
Ping. |
|
@rwjblue anything I can do to get this merged? Thank you! |
|
We're about to start using sourcesets in production and we will need to prepend the bucket's url, so we also need this fix. |
|
@rickharrison can you help us? |
|
Would anyone be interested in becoming a maintainer of this project? I am unable to test this change at the moment due to overwhelming obligations at work. |
|
I can do so.
…On 25 September 2018 at 00:21:39, Rick Harrison ***@***.***) wrote:
Would anyone be interested in becoming a maintainer of this project? I am
unable to test this change at the moment due to overwhelming obligations at
work.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#61 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AACNfW02OszNrqWN8A4_FhB58IJg8ADpks5ueVrzgaJpZM4PvZOc>
.
|
|
@rickharrison is your offer still valid? I can be a maintainer for this project, can take broccoli-asset-rev too, as they are quite related if you want. Thank you! |
|
Closing this. |
|
Nooo |
|
@Serabe @rickharrison can this issue be revived/reopened? |
|
I tried over two years to get this merged. I'm not interested on further trying unless any of the maintainers show any kind of intention to help moving this forward. |
|
@rickharrison, is it possible to transfer repo to https://github.com/broccolijs org? |
The problem in ember-cli/broccoli-asset-rev#48 happened because
the logic to avoid prepending assets twice did not take into account
the edge case of srcset, where there are many assets in one match.
This edge case complicates things more than a bit. To simplify the
solution, when a prepend option is found, we use a replacer function
as second parameter.
Closes ember-cli/broccoli-asset-rev#48
Closes #51