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

fix(css): using background url which match alias in sass/less, there will be link error (fix #2316) #2323

Merged
merged 4 commits into from
Mar 24, 2021

Conversation

crcong
Copy link
Contributor

@crcong crcong commented Mar 1, 2021

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)

@crcong
Copy link
Contributor Author

crcong commented Mar 5, 2021

The error prompt is shown in the figure below.

WX20210305-233543@2x

After I run vite build, I find that the URL link is normal, so I think it's a bug.

Maybe my fix way is not very elegant, I hope someone can guide me. Thanks.

@crcong crcong changed the title fix(css): rewriteCssUrls abnormal when has alias (fix #2316) fix(css): using background url which match alias in sass/less, there will be link error (fix #2316) Mar 5, 2021
@@ -848,6 +851,7 @@ type StylePreprocessor = (
[key: string]: any
additionalData?: PreprocessorAdditionalData
filename: string
alias: Alias[]
Copy link
Member

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)

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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 😅

@patak-dev patak-dev added the p3-minor-bug An edge case that only affects very specific usage (priority) label Mar 20, 2021
@patak-dev patak-dev requested a review from antfu March 20, 2021 18:26
@patak-dev patak-dev merged commit 9499d26 into vitejs:main Mar 24, 2021
crcong added a commit to crcong/vite that referenced this pull request Mar 24, 2021
@crcong crcong deleted the fix-alias branch March 24, 2021 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

alias BUG
4 participants