-
Notifications
You must be signed in to change notification settings - Fork 6
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
Existing URL parameters are removed #122
Existing URL parameters are removed #122
Comments
Hey there @angeloashmore, thanks for opening this issue. So this is a peculiar issue which we had already slated to discuss internally next week since it arose in one of our other libraries. Essentially this is not as simple as "just make the thing work" since there are some edge-cases here that we have to handle with regards to URL encoding. In short I don't have a resolution here for you yet, but I will update you once we have discussed with the team next week. |
Awesome, thank you! |
Hey there @angeloashmore! We discussed this as a team and unfortunately we are not going to fix this due to how we handle URL encoding. Essentially, the I hope this makes sense! We discussed this at length yesterday including some other options, including:
In saying this, there are two bugs that we have seen in this issue, namely that a) the path should not be truncated at the I'm going to close this issue but please feel free to comment if you have any suggestions or ideas. I have to say we're pretty set on this decision but are always open to consider other legitimate ideas. |
Hey @frederickfogerty, thanks for the detailed update. Totally understandable, and I'm sure there are edge cases I can't imagine myself. The specific issue I have is something that, I think, can be handled at the consumer level (in this case, Re: the |
Glad that we could work this out and glad that it can hopefully be handled at the consumer level. Let me know if you'd like to brainstorm any ideas about it together, and please let me know what you come up with, as it's probably a problem that other users of this library will run into, so I'd love to add it to the README. Re: the |
Hey @frederickfogerty, I was able to come up with something that seems to work in most cases. I'll explain why I say most later. This function takes in a field config created via All tests are passing except for Here is a failing test: https://github.com/angeloashmore/gatsby-source-prismic/blob/124b054bbff5ac5b6ceba103e494b580fd37f6ce/packages/gatsby-source-prismic/test/create-schema-customization-image.test.ts#L346-L349 Basically, if someone sets a This happens here: gatsby/src/modules/gatsby-plugin/buildGatsbyImageDataBaseArgs.ts Lines 18 to 20 in 32364af
Would you be open to swapping the order of the property spreading? It would become this: const src = client.buildURL(imageName, {
...(typeof opts.imgixParams === 'object' && opts.imgixParams),
w: width,
h: height,
}); This matches the approach taken with the gatsby/src/modules/gatsby-plugin/objectBuilders.ts Lines 158 to 164 in 32364af
|
Opened a PR so it can be merged in easily if this change is okay: #125 |
Thanks for the explanation @angeloashmore, this sounds like a good solution! I'll respond to the width and height override over on that PR. |
Great, thanks. With that PR, I think I'm all good with the Prismic integration. |
Describe the bug
If a URL has existing URL parameters, such as
rect
for cropping, they are removed after the URL is transformed forurl
,fluid
,fixed
, andgatsbyImageData
fields. Only parameters given todefaultImgixParams
, the GraphQL field's arguments, and those produced by the library itself (such as during srcSet generation) are included.To Reproduce
Provide a URL to the plugin with a
rect
parameter. UsingbuildFixedImageData
as an example:The following is logged:
Note the exclusion of
?auto=compress,format&rect=0,1100,6091,1865
in the URLs, which may be crucial to the image's representation.Expected behaviour
The URL should include original parameters. Any parameters added by the plugin should be merged over the existing params.
Information:
1.6.3
Additional context
I believe this is the behavior of
@imgix/js-core
'sbuildURL
method. If the core JS library is to be left as is, could the Gatsby plugin automatically read the URL parameters and merge them into the top of the final params?For example, the URL field config could add
...existingUrlParams
abovedefaultParams
here:gatsby/src/modules/gatsby-source-url/createImgixUrlFieldConfig.ts
Line 57 in 8f921b6
See: https://github.com/imgix/js-core/blob/e168e5da6874591f657ce6de95257fe5414a361c/src/index.js#L47-L60
The text was updated successfully, but these errors were encountered: