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

Strip query strings and hashes when finding assets #119

Merged
merged 2 commits into from Oct 2, 2019

Conversation

@lencioni
Copy link
Contributor

commented Oct 2, 2019

Happo has some logic that looks for any local assets needed to render
the examples in a browser (e.g. images), and tries to bundle them up so
they can be available during screenshotting.

We've recently noticed a bug where some of our local images are failing
to load. When viewing source in the Happo UI, these images are 404ing,
and they do not exist in the assets zip file.

We noticed that these local images have some query strings added to
them. We believe that these query strings are causing these images to
not be bundled up with the other assets.

In this commit, I am hoping to make this better by stripping query
strings from the URLs. I started by trying to use node's URL built-in
parsing, but I found a couple of issues:

  1. The new URL() API does not support relative URIs. There are some
    workarounds for this, like using a base of relative:///, but I was
    a bit worried about what unexpected consequences would arise from
    going this route. More information:
    nodejs/node#12682
  2. The url.parse() API is deprecated, and it gives you a URL object to
    work with. I wanted to essentially preserve the original format
    without adding any new information and I was a bit worried about
    getting that out of this object cleanly.

So in the end, I decided to write a simple regex for this.

My hunch is that this is an okay fix for now, but I imagine there might
be some situations where the query string is important for local images
and needs to be preserved in a better way so that all of the images are
correctly served. For example, imagine these two image URLs:

  • ./foo.jpg?size=large
  • ./foo.jpg?size=small

If these are used in an srcset, after this change, we will only bundle
up a single ./foo.jpg image and that will be served for both.

I think the fix for this is a bit more involved (e.g. modifying the
image URLs so that the query strings are actually part of the filename,
or maybe storing them in a way that the server is able to pick the right
one when rendering examples for snapshotting), and it is unclear to me
if it actually matters at this time, so I'm calling YAGNI.

@lencioni lencioni requested a review from trotzig Oct 2, 2019
Happo has some logic that looks for any local assets needed to render
the examples in a browser (e.g. images), and tries to bundle them up so
they can be available during screenshotting.

We've recently noticed a bug where some of our local images are failing
to load. When viewing source in the Happo UI, these images are 404ing,
and they do not exist in the assets zip file.

We noticed that these local images have some query strings added to
them. We believe that these query strings are causing these images to
not be bundled up with the other assets.

In this commit, I am hoping to make this better by stripping query
strings from the URLs. I started by trying to use node's URL built-in
parsing, but I found a couple of issues:

1. The `new URL()` API does not support relative URIs. There are some
   workarounds for this, like using a base of `relative:///`, but I was
   a bit worried about what unexpected consequences would arise from
   going this route. More information:
   nodejs/node#12682
2. The `url.parse()` API is deprecated, and it gives you a URL object to
   work with. I wanted to essentially preserve the original format
   without adding any new information and I was a bit worried about
   getting that out of this object cleanly.

So in the end, I decided to write a simple regex for this.

My hunch is that this is an okay fix for now, but I imagine there might
be some situations where the query string is important for local images
and needs to be preserved in a better way so that all of the images are
correctly served. For example, imagine these two image URLs:

- ./foo.jpg?size=large
- ./foo.jpg?size=small

If these are used in an srcset, after this change, we will only bundle
up a single `./foo.jpg` image and that will be served for both.

I think the fix for this is a bit more involved (e.g. modifying the
image URLs so that the query strings are actually part of the filename,
or maybe storing them in a way that the server is able to pick the right
one when rendering examples for snapshotting), and it is unclear to me
if it actually matters at this time, so I'm calling YAGNI.
@lencioni lencioni force-pushed the lencioni:asset-query-strings branch from 07eb512 to fd0adf8 Oct 2, 2019
@@ -17,6 +17,11 @@ it('finds images', () => {
expect(subject()).toEqual([{ assetPath: '/1x1.png', resolvePath: '/1x1.png' }]);
});

it('strips query strings', () => {
css = '.body { background-image: url("/1x1.png?pizza=yum");';
expect(subject()).toEqual([{ assetPath: '/1x1.png', resolvePath: '/1x1.png' }]);

This comment has been minimized.

Copy link
@lencioni

lencioni Oct 2, 2019

Author Contributor

@trotzig I wasn't entirely sure if the query string should be stripped for both assetPath and resolvePath

This comment has been minimized.

Copy link
@trotzig

trotzig Oct 2, 2019

Contributor

You did the right thing.

src/stripQueryString.js Outdated Show resolved Hide resolved
This is much less likely than query strings, but we may as well do this
to be extra careful in case it does come up.
@lencioni lencioni changed the title Strip query strings when finding assets Strip query strings and hashes when finding assets Oct 2, 2019
@trotzig
trotzig approved these changes Oct 2, 2019
Copy link
Contributor

left a comment

Thanks for the fix!

@@ -17,6 +17,11 @@ it('finds images', () => {
expect(subject()).toEqual([{ assetPath: '/1x1.png', resolvePath: '/1x1.png' }]);
});

it('strips query strings', () => {
css = '.body { background-image: url("/1x1.png?pizza=yum");';
expect(subject()).toEqual([{ assetPath: '/1x1.png', resolvePath: '/1x1.png' }]);

This comment has been minimized.

Copy link
@trotzig

trotzig Oct 2, 2019

Contributor

You did the right thing.

@trotzig trotzig merged commit c10a075 into happo:master Oct 2, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lencioni lencioni deleted the lencioni:asset-query-strings branch Oct 2, 2019
@trotzig

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.