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

Support double quotes in use imports from built-ins #147

Merged
merged 2 commits into from Jul 2, 2021

Conversation

amannn
Copy link
Contributor

@amannn amannn commented Jun 23, 2021

@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 Reviewable

@@ -24,7 +24,7 @@ export default function rewritePaths(error, file, contents, moduleContext, callb

const rewritten = contents.replace(useRegexp, (entire, importer, single, double, unquoted) => {
Copy link
Contributor Author

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?

Copy link
Member

Choose a reason for hiding this comment

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

No idea.

@amannn amannn marked this pull request as ready for review June 23, 2021 08:51
@justin808
Copy link
Member

Thanks for the contribution.

  1. Can you make sure the new code you introduced has some examples in the example directory. If appropriate, add a new example.
  2. Does this require some doc change?

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)) {
Copy link
Member

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.

Copy link
Member

@Tomburgs Tomburgs left a 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.

@justin808
Copy link
Member

@amannn, @Tomburgs Should we merge/release in your opinions? or wait for improvements? See my question above.

@justin808 justin808 merged commit c391d6b into shakacode:master Jul 2, 2021
@justin808
Copy link
Member

Should we let folks try the master branch before I push out a release?

Thanks @amannn and @Tomburgs!

@amannn
Copy link
Contributor Author

amannn commented Jul 2, 2021

Thanks a lot for merging this @justin808!

@justin808
Copy link
Member

@amannn I'll push the release when you give me the 👍 .

@amannn
Copy link
Contributor Author

amannn commented Jul 2, 2021

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:

SassError: SassError: There's already a module with namespace "math".
    ╷
1   │ @use 'sass:math';
    │ ━━━━━━━━━━━━━━━━ original @use
... │
6   │ @use "sass:math";
    │ ^^^^^^^^^^^^^^^^ new @use

It's currently not an issue for me though.

@amannn amannn changed the title Supported double quoted use imports from built-ins Support double quotes in use imports from built-ins Aug 5, 2021
@justin808
Copy link
Member

Shipped!

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.

None yet

3 participants