Use the remapped URI for the cache key (#1818)#2109
Conversation
katrinafyi
left a comment
There was a problem hiding this comment.
After some thinking, the approach is sound.
Since this adds a new call to remap, I was worried that this would be applying the remap to a URL which was already remapped. It does do two remaps, but both remaps are done from the initial URL so it should be sound, if a touch inefficient.
There's another PR concerning remaps at #2094, but I think it shouldn't conflict.
Thanks for looking into this!
There was a problem hiding this comment.
@thomas-zahner do you have thoughts about this approach, in particular after #2094? I don't love repeating the remap separately outside of check(). if we hypothetically add more pre-check transforms, they would also have to be duplicated for the cache key. however, it is hard to extract the remapped URL from check() for use as the cache key, because it does the remap then immediately does the check.
so it's a bit tricky. I would be okay with merging this pr as-is and keeping these issues in the back of our mind. but what do you think?
|
@katrinafyi Thanks for the feedback! I have removed the redundant
I agree. Though I didn't see an easy way to move the remap further up without a larger refactor so I settled on the current approach. I'm not sure why |
|
Yes I agree that it's fine to remap twice.
Additionally, I tried to clean some things up, not directly related to this PR in 9fd5596. |
I see, thanks for pointing out.
Many thanks! |
|
@thomas-zahner Can we revert 9fd5596? It might be worthwhile, but I think it's too much to tack onto the tail of an otherwise small PR. Otherwise, lgtm. Thanks @josrepo! |
c1288fb to
59a4e8b
Compare
Yeah makes sense 👍 |
|
Thank you @josrepo for the nice contribution! 🚀 |
Fixes and adds a test for #1818.