-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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 missing whitespace issue in srcsets #3132
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test case?
It is somewhat hard to test this explicitly, but I changed to snapshot to expect a whitespace between the import URL and the descriptor. This makes the tests pass + reflects how the srcset works. |
A new test that checks the generated srcset would be better to avoid a regression |
I get that, how am I supposed to test the generated srcset though? Since all I get is the intermediate AST, I can either test the ASTs properties or to a string comparison with the generated string. If I'm supposed to do the test via AST checking, what properties should I assert? The hoists seems pretty reasonable to me but I'm not familiar with the AST structure. |
@JonasKruckenberg In this scenario, a more reliable test would be testing the output html rather than the AST to ensure there is only one space. For instance, having the snapshot completely overlooked the problem you reported. So I think a nonregression test case inside of vue's index.spec.ts file would help avoiding it failing in the future with a clear error. It would be nice to also add a comment referencing this PR |
This pull requests fixes issues #3069, #1626 (as well as #1587) where the generated srcset would be invalid because the space between url and descriptor was missing. This PR aims at resolving this as it makes secret basically unusable.
The solution is to simply add a space in front and has already been used on line 81 so I think this should be really uncontroversial.
Cheers!