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
refactor: isCSSRequest for all css #4484
Conversation
@@ -147,7 +151,10 @@ export function transformMiddleware( | |||
|
|||
// for CSS, we need to differentiate between normal CSS requests and | |||
// imports | |||
if (isCSSRequest(url) && req.headers.accept?.includes('text/css')) { | |||
if ( | |||
isIndirectCSSRequest(url) && |
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 think we shouldn't use isIndirectCSSRequest
here because at this point we don't know if it is direct or not. It would be more clear to me if we use isCSSRequest(url) && !isDirectRequest(url)
This should read as "is this a CSS Request that is not yet marked as direct?"
This PR is modifying some calls that we should review:
@OneNail is this analysis correct? Would you help us with comments about why each of these changes is correct? |
If vite/packages/vite/src/node/plugins/css.ts Line 105 in 690b35e
That looks fine to me. I wonder why it was added. It was part of the original commit that added the direct check. |
@patak-js Thank you very much for your review, I think your analysis is correct. So it seems that
|
if (isCSSRequest(url) && req.headers.accept?.includes('text/css')) { | ||
if ( | ||
isCSSRequest(url) && | ||
!isDirectCSSRequest(url) && |
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 think we should use a direct regex check here or maybe add a isDirectRequest(url)
function to avoid repeating the CSS regex on every CSS file.
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 misunderstood it before and have changed it
Description
#4127 (comment)
isCSSRequest
is misleading. This can easily cause some bugs as people may using it without knowing that it explicitly tests for not being a direct request.Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).