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
Support double quotes in use imports from built-ins #147
Conversation
@@ -24,7 +24,7 @@ export default function rewritePaths(error, file, contents, moduleContext, callb | |||
|
|||
const rewritten = contents.replace(useRegexp, (entire, importer, single, double, unquoted) => { |
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.
Not sure what unquoted
is, or if we should support this as well?
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.
No idea.
Thanks for the contribution.
Can you try to find another reviewer familiar with these changes? @Tomburgs can you help? |
@@ -24,7 +24,7 @@ export default function rewritePaths(error, file, contents, moduleContext, callb | |||
|
|||
const rewritten = contents.replace(useRegexp, (entire, importer, single, double, unquoted) => { | |||
// Don't rewrite imports from built-ins | |||
if (single && single.indexOf('sass:') === 0) { | |||
if ([single, double].some(match => match && match.indexOf('sass:') === 0)) { |
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.
Could we match the quotes into a separate group and become completely indifferent to them @justin808?
The regex could be a bit tricky, as for example this might be a valid import @use 'file/with"Quotes"'
, but I believe backreference should be all that we need, something like /('|")(text-group-match)\1/
, so that the last \1
group would match only the same symbol that the first group had already matched.
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.
It'd be nice to improve the regex match which I noted in the previous comment, but overall it looks good to me.
Thanks a lot for merging this @justin808! |
@amannn I'll push the release when you give me the 👍 . |
I've just tried this out in my project and it works both with single and double quotes for me. However what I did see, is that you can get an error when you have mixed quotes:
It's currently not an issue for me though. |
Shipped! |
@justin808 Related to #142, I noticed double quotes need to be handled separately.
Also sorry for the subtle bug that slipped in in #142!
This change is