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
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/utils/rewritePaths.js
Expand Up @@ -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.

// 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.

return entire;
}

Expand Down
18 changes: 18 additions & 0 deletions test/__snapshots__/index.test.js.snap
Expand Up @@ -103,6 +103,20 @@ exports[`sass-resources-loader imports should not rewrite the path for built-ins
@use 'sass:math';


$padding: #{math.div(4 / 2)}px;

div {
padding: $padding;
margin: #{math.div(4 / 2)}px;;
}
"
`;

exports[`sass-resources-loader imports should not rewrite the path for built-ins with @use and double quotes 1`] = `
"// Should be de-duplicated
@use 'sass:math';


$padding: #{math.div(4 / 2)}px;

div {
Expand Down Expand Up @@ -140,6 +154,10 @@ exports[`sass-resources-loader resources should parse array resources 1`] = `

@forward \\"variables\\";

@use \\"sass:math\\";

$padding: #{math.div(4 / 2)}px;

@use 'sass:math';

$padding: #{math.div(4 / 2)}px;
Expand Down
11 changes: 11 additions & 0 deletions test/index.test.js
Expand Up @@ -252,6 +252,17 @@ describe('sass-resources-loader', () => {
const output = require('./output/use-builtin').default;
expect(output).toMatchSnapshot();
}));

it('should not rewrite the path for built-ins with @use and double quotes', () => execTest('use-builtin', {
hoistUseStatements: true,
resources: [
path.resolve(__dirname, './scss/shared/_math-double-quotes.scss'),
],
}).then(() => {
// eslint-disable-next-line global-require
const output = require('./output/use-builtin').default;
expect(output).toMatchSnapshot();
}));
});

describe('getOutput', () => {
Expand Down
3 changes: 3 additions & 0 deletions test/scss/shared/_math-double-quotes.scss
@@ -0,0 +1,3 @@
@use "sass:math";

$padding: #{math.div(4 / 2)}px;