-
Notifications
You must be signed in to change notification settings - Fork 115
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
Allow urls that contain fragments, fixes #560 #561
Allow urls that contain fragments, fixes #560 #561
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.
Thank you! This test case covers the Vite use case 👍
@RyanZim could you take a look at this when you have time? |
Apologies, I've been incredibly busy. This is technically a breaking change, right? Code LGTM. |
I don't think this is a breaking change :) Import statements with fragments were ignored before this change. This would be a silent failure. After this change This is no longer a silent failure but users can trivially resolve this by removing the incorrect I changed it in v14 was because it is more correct in a bundler context when only considering the CSS specification. I wasn't aware of how tools like Vite used this. Edit: Always possible that I overlooked something :) |
Thanks for the detailed explanation. I think it's still technically a breaking change, key word being technically, but I agree it probably doesn't affect any real-world use, so probably safe to release without a major bump. |
I'm a fairly strong believer in "never ship on Fridays," so will be sometime next week until this is released. |
Published v16.1.0 |
Thank you @RyanZim 🙇 |
The intention behind ignoring urls with a fragment was to make the plugin more correct.
The thinking at the time was that these are never valid imports for a filesystem.
With nodejs subpath imports that assumption is incorrect.