Make strict mode optional (but enabled by default) for both .parse() and .stringify()#13
Conversation
This adds an optional second param named options to the .parse() method. Currently the only option is `strict` which defaults to `true` unless explicitly set to `false` Setting strict mode to `false` prevents it from throwing errors on invalid input, and it instead it simply returns a "best guess". This also adds several additional checks when strict mode is enabled: * no more than one image candidate for any given descriptor * no more than one descriptor per image candidate * height descriptor gets an explicit error message (or is silently accepted when strict mode is disabled) Because of the duplicate check, the deepUnique function is no longer needed when strict mode is enabled, and IMO undesired when strict mode is disabled. Because of this, I removed it. I also tweaked the existing tests to remove invalid input and added a set of new tests with intentionally invalid input to ensure it throw when strict mode is enabled and does not throw when disabled.
This adds an optional second param named options to the .stringify() method. Currently the only option is `strict` which defaults to `true` unless explicitly set to `false` Setting strict mode to `false` prevents it from throwing errors on invalid input, and it instead it simply returns a "best guess". I refactored the existing checks into a few standalone methods so that the same checks can be run for both .parse() and .stringify(). In some cases, the error messages were changed slightly. I removed the set from the .stringify() method, allowing for duplicate image candidates when strict mode is disabled (it is prevented by the check when enabled). I also tweaked the existing tests to remove invalid input and added a set of new tests with intentionally invalid input to ensure it throw when strict mode is enabled and does not throw when disabled.
Do you plan to do that in this pull request? |
|
Cool, I'll get this cleaned up today.
Probably not this PR, I feel like there's enough going on here already. One more thing I would like to see changed is remove the default |
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
* update index.d.ts to cover new options and match readme * use destructuring for options * use undefined instead of null * use for-of loop instead of forEach * use .notThrows in test
|
Ok, I think I covered everything, although I'm not sure if the readme is what you had in mind or not. |
index.d.ts
Outdated
| /** | ||
| * @deprecated height is no longer allowed in the srcset specification | ||
| */ | ||
| height?: number; |
There was a problem hiding this comment.
I don't think we should add this. It was already absent, so TS users would not have used it.
There was a problem hiding this comment.
I kind of disagree with you here, but OTOH typescript has escape hatches if someone really wants to use height. I'll nix it.
|
Ok, how's this look? |
| const invalidStrings = [ | ||
| 'banner.jpeg, fallback.jpeg', // Multiple fallback images | ||
| 'banner-phone-HD.jpg 100w 2x', // Multiple descriptors | ||
| 'banner-HD.jpeg 2x, banner.jpeg 2x', // Multiple images with the same descriptor |
There was a problem hiding this comment.
I think some of the changes in this PR can be considered breaking changes. I can understand a loose mode, but I'm not sure when a strict mode would be helpful? IMHO, we should try to follow the resolution logic of the browser as much as possible. For instance, Chrome doesn't throw any errors when there are multiple images with the same descriptor. Instead, it simply select the first one.
A stricter logic can be implemented in a separated function validate instead, so that we can simplify the source code here, and also allowing users to have more controls if needed.
At the very least, I don't think we should make this strict mode enabled by default. Users with valid (valid as in compatible with most browsers) code in the browsers might experience unexpected errors when using this library.
That's just my 2 cents though. Curious to hear your thoughts :)
There was a problem hiding this comment.
Yea, I don't disagree with you in general, this probably should be considered a breaking change.
Prior to this PR, .parse() is strict and .stringify() is loose. I prefer strict mode by default, although I actually want the loose mode for my use-case, but mainly just wanted it to be configurable.
Also, I think the most common use case for this library is to parse, then modify, then stringify. In that case since .parse() was already strict, making both .parse() and .stringify() strict by default seems like less of a change than making them both loose by default.
I considered making the default match the prior behavior, but rejected that because I didn't like the inconsistency.
There was a problem hiding this comment.
I think the most common use case for this library is to parse, then modify, then stringify.
As you said it here, making it more strict than the current implementation immediately makes it less useful, doesn't it? Since that we're most likely parsing the strings from HTML in the browsers. I agree that parse and stringify should behave more or less the same, but I don't think that we should change the current implementation to throw on some of these cases here. Could we only update stringify here to match the behaviors in parse?
There was a problem hiding this comment.
making it more strict than the current implementation immediately makes it less useful, doesn't it?
I don't think so.
- I think that for some use cases you want things to be validated, and then you want all of the checks.
- In other cases you just want it to give a best effort and not throw errors.
I believe this is an improvement for both of those cases.
I can't think of many situations where you'd want some things to be validated but not others (?) I suppose if that came up, you could just turn off validation in this library and do it yourself. Prior to this change, though, that also wasn't an option, so this still feels like an improvement.
Am I missing something?
I don't think that we should change the current implementation to throw on some of these cases here
Which cases in particular? The only really new one is "no more than one image candidate for any given descriptor". (So, no foo.jpg 2x, bar.jpg 2x.)
I just edited my original description, because "no more than one descriptor per image candidate" was sort of there already. foo.jpg 100w 2x would throw an error, but foo.jpg 100w 200w wouldn't.
There was a problem hiding this comment.
My sole point here is that we shouldn't throw errors for srcsets that are totally working on most modern browsers, at least not by default. I agree we can make a strict mode and a loose mode, but I don't think the strict mode should be enabled by default. That is, if we can make this option defaults to false then I'm all in for the changes 👍.
Which cases in particular? The only really new one is "no more than one image candidate for any given descriptor". (So, no foo.jpg 2x, bar.jpg 2x.)
Yep, this is the one. I don't know if there's any other cases though, I haven't done any testing 😅. Maybe we can make a table to test some modern browsers implementation to find out what the "sensible defaults" are? (That's a lot of work though 😅, should probably do this in another PR/issue)
There was a problem hiding this comment.
I don't think the strict mode should be enabled by default
Ok, that's easy enough to change.
As for what browsers do, I did a bit of testing:
- Chrome and Firefox will both accept
100h 100w- the current version of the library will throw an error on this - Chrome logs errors on some invalid image candidates:
Failed parsing 'srcset' attribute value since it has an 'h' descriptor and no 'w' descriptor.Failed parsing 'srcset' attribute value since it has multiple 'x' descriptors or a mix of 'x' and 'w'/'h' descriptors.
- Chrome silently ignores errors where multiple image candidates have the same descriptors (it just never uses any but the first image for any descriptor.)
- Firefox never logs any errors
- They have some difference in which image they choose - Chrome seems to prefer 2x images even on 1x displays, whereas Firefox is better at matching the image to the display density. (Chrome's behavior could be because I had a different display that was 2x attached. I didn't test it with only a 1x display.)
There was a problem hiding this comment.
I just now switched it to loose mode by default. (Perhaps "permissive mode" is a better name?)
also improve duplicate detection to disallow 1x + fallback and things like 2.0x + 2x
I finished this. I'm starting to rethink that "Probably not this PR" comment. The changes are currently in a separate PR: nfriedly#1 - but that targets the branch for this PR so that they can all be merged in together if we merge that one and then this one. |
Remove default density: 1 from fallback images, improve duplicate validation
also removed duplicate tests
|
Are there any breaking changes in this PR now? |
It no longer de-duplicates by default, and it doesn't set It's debatable, but I'd lean towards making it a semver major release. |
There's a lot going on here, but I think it all makes sense and fits together:
The headline feature is that
.parse()and.stringify()both now accept an optional second argument for options. The only current option isstrictwhich defaults totruefalse.This might be considered a breaking change for.stringify()since it previously worked in "loose mode". I suppose we could change the default tofalsethere if you prefer..parse()I also added several additional checks when strict mode is enabled:
100w 2x, but it would allow two of the same, e.g.100w 200w.Because of the duplicate check in strict-mode, I removed the
deepUniquefunction that was used in.parse()and theSetthat was used in.stringify(). In non-strict-mode, it makes sense to allow duplicates IMO.Finally, I tweaked a few of the existing tests to ensure they passed in strict mode, and then added a handful of invalid fixtures and tests to ensure they throw in strict mode but not in non-strict-mode.
One thing I haven't dug into yet is equating the fallback image (with no descriptor) toUpdate: this is now included in this PR via nfriedly#11x- right now the validator treates those as two different things and therfore allows something likea.jpg 1x, b.jpgto pass. It also considers1xand1.0xto be different. So there's room for further improvement, but I still think this is a step in the right direction.