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

fix: ensure undefined parameters not added to URL #286

Merged
merged 2 commits into from
Jun 15, 2021
Merged

Conversation

luqven
Copy link
Contributor

@luqven luqven commented Jun 3, 2021

This PR ensures that undefined parameters are not added to the
params object generated by _buildParams. This way, undefined params do
not then end up in the generated URL.

@luqven luqven requested a review from a team as a code owner June 3, 2021 14:16
@commit-lint
Copy link

commit-lint bot commented Jun 3, 2021

Tests

  • ensure undefined parameters not present in URL (85d054c)

Bug Fixes

  • ensure undefined parameters not added to url (f7fc708)

Contributors

luqven

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

  • @Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
  • @Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
  • @Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
  • @Commit-Lint merge disable will desactivate merge dependabot PR
  • @Commit-Lint review will approve dependabot PR
  • @Commit-Lint stop review will stop approve dependabot PR

Copy link
Contributor

@sherwinski sherwinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my context, would you mind explaining what the motivation for this PR is? In isolation, it seems fine but I'm hesitant to add guardrails for scenarios that don't sound likely to happen?

That is, unless this is proactively guarding against something that we know is happening, such as in a consuming library.

@frederickfogerty
Copy link
Contributor

frederickfogerty commented Jun 4, 2021

@sherwinski there was a bug occurring in Gatsby that was caused by this. Basically, we had some code like this:

const imgixParams = {
  ...args.imgixParams,
  ar: (width != null && height != null) ? `${width/height}:1` : undefined
};

const src = client.buildURL(path, imgixParams); // => contains ar=undefined

You can see ar=undefined showing up in the URLs from a user's bug report here: imgix/gatsby#122

I think this is pretty common not only to have undefined values be set by our libraries, but also being passed down from SDK consumers. Thus I would say this has a high impact.

It's a pretty common pattern that if an object value is undefined or null it should be treated as if it is not set. To workaround this we would have to manually prune all null and undefined from the imgixParams object before passing it to js-core in all of our libraries.

Copy link
Contributor

@frederickfogerty frederickfogerty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sherwinski
Copy link
Contributor

sherwinski commented Jun 4, 2021

@frederickfogerty I guessed as much, but figured I ask anyways 😁

luqven and others added 2 commits June 15, 2021 13:10
Co-authored-by: Frederick Fogerty <frederick@imgix.com>
This commit ensures that undefined parameters are not added to the
params object generated by _buildParams. This way, undefined params do
not then end up in the generated URL.

Co-authored-by: Frederick Fogerty <frederick@imgix.com>
@luqven luqven merged commit f7e6118 into main Jun 15, 2021
@luqven luqven deleted the f/removeUndefined branch June 15, 2021 20:12
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.

None yet

3 participants