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 parsing of srcset without whitespaces #11

Merged
merged 4 commits into from
Apr 19, 2021

Conversation

kevin940726
Copy link
Contributor

srcset without whitespaces like banner-HD.jpeg 2x,banner-HD.jpeg 2x,banner-HD.jpeg 2x,banner-phone.jpeg 100w,http://site.com/image.jpg?foo=100w,lorem 1x cannot be parsed. I rewrote the split logic and refactor the code to match more closely to the spec. Below are some added rules:

  • The width descriptor must not be a float number.
  • The value of the descriptor must be a valid number.
  • An image candidate string with no descriptors is equivalent to an image candidate string with a 1x descriptor.

Also added some tests.

@sindresorhus sindresorhus changed the title Fix srcset without whitespaces cannot be parsed Fix parsing of srcset without whitespaces Apr 18, 2021
index.js Outdated Show resolved Hide resolved
index.js Outdated
@@ -1,6 +1,6 @@
'use strict';

const integerRegex = /^-?\d+$/;
const srcsetRegex = /\s*([^,]\S*[^,](?:\s+[^,]+)?)\s*(?:,|$)/;
Copy link
Owner

Choose a reason for hiding this comment

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

Why such a complicated regex? If it really has to be like this, I would like to see a code comment that explains it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed the regex to imageCandidateRegex to indicate that it matches loosely to an "image candidate string". It's the simplest way I can think of for now to solve the issue where there's no whitespace between two image candidate strings. I've included some comments above. Let me know if I can make it clearer!

@sindresorhus sindresorhus merged commit 201a7da into sindresorhus:main Apr 19, 2021
@kevin940726 kevin940726 deleted the fix/refactor-split branch April 20, 2021 01:45
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.

2 participants