-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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(css): using background url which match alias in sass/less, there will be link error (fix #2316) #2323
Conversation
@@ -848,6 +851,7 @@ type StylePreprocessor = ( | |||
[key: string]: any | |||
additionalData?: PreprocessorAdditionalData | |||
filename: string | |||
alias: Alias[] |
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.
nitpick (non-blocking): Plural of alias
is aliases
(applies also to other lines)
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 for your advice. I've changed the name of alias
to aliases
.
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.
Sorry for going against this change, but we are using alias
in other config options, and it is a string of aliases https://vitejs.dev/config/#resolve-alias. It is strange in the code to see aliases = config.resolve.alias
. I think the singular form was ok here. @Shinigami92 @crcong what do you think?
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.
Mh I complained about it, because it was passed and always used as an array (in this PR)
In convention perspective this would mean that it should be named in plural form
Keep in mind that it is only a code change right now and not a breaking change, so config.resolve.alias
is still config.resolve.alias
in the public exposed API
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 found that the configuration config.resolve.alias
has been processed into an array inside vite
, so I also think it is ok to use the singular here. I will change it back. @Shinigami92 @matias-capeletto Thank you for your suggestions.
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.
Okay, sorry for the inconvenience 😅
vitejs#2316) (vitejs#2323)" This reverts commit 9499d26.
After testing in playground, I found that when using background url which contains alias in sass/less, there will be link error scenarios.(fix #2316)