-
-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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: allow passing an array as sass / scss importer #3529
Conversation
This change allows passing an array with multiple custom importers. JavaScript does not mind passing `undefined` to concat, so additional checks are not needed. In that case shallow copy is created, but it does not matter here. Closes vitejs#3526.
? importer.concat(options.importer) | ||
: importer.push(options.importer) | ||
} | ||
const importer = [internalImporter].concat(options.importer!) |
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.
If options.importer
would be undefined
here, this would add an undefined
value to the array 🤔
Do we really want this non-null assertion here?
Beside that, does this really fix the issue? And if so: why?
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.
I was thinking on something like this to fix the issue:
const importer = [internalImporter]
if (options.importer) {
if( Array.isArray(options.importer) ) {
importer.push(...options.importer)
}
else {
importer.push(options.importer)
}
}
This could be written in a more compact form but I don't think we will gain in readability.
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.
@patak-js So your solution is exactly the same as before but using push
instead of concat
?
? importer.concat(options.importer) | ||
: importer.push(options.importer) | ||
} | ||
const importer = [internalImporter].concat(options.importer!) |
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.
I was thinking on something like this to fix the issue:
const importer = [internalImporter]
if (options.importer) {
if( Array.isArray(options.importer) ) {
importer.push(...options.importer)
}
else {
importer.push(options.importer)
}
}
This could be written in a more compact form but I don't think we will gain in readability.
Well, you're right :) Anyway, here is another attempt. |
Description
This change allows passing an array with multiple custom importers. Fixes #3526.
Additional context
JavaScript does not mind passing
undefined
to concat, so additional checks are not needed. In that case shallow copy is created, but it does not matter here.What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).