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(plugin/assets-retry): addQuery and switchDomain should work in async css chunk #4315

Merged
merged 9 commits into from
Jan 5, 2025

Conversation

SoonIter
Copy link
Member

@SoonIter SoonIter commented Jan 2, 2025

Summary

fix

    // At present, we don't consider the switching domain and addQuery of async CSS chunk
    // 1. Async js chunk will be requested first. It is rare for async CSS chunk to fail alone.
    // 2. the code of loading CSS in webpack runtime is complex and it may be modified by cssExtractPlugin, increase the complexity of this plugin.

When I implemented this plugin, rsbuild was just switched from experiment.css to cssExtractPlugin experimentally, I didn't know much about cssExtract.

after this change, it means that plugin-assets-retry can only be used together with rspack.CssExtractPlugin and MiniCssExtractPlugin in webpack

😂 by the way, it was also not implemented in "eden"

Related Links

close #4306

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

Copy link

netlify bot commented Jan 2, 2025

Deploy Preview for rsbuild ready!

Name Link
🔨 Latest commit 3084c96
🔍 Latest deploy log https://app.netlify.com/sites/rsbuild/deploys/677652029606b50008c90fd8
😎 Deploy Preview https://deploy-preview-4315--rsbuild.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 70 (🟢 up 1 from production)
Accessibility: 97 (no change from production)
Best Practices: 100 (no change from production)
SEO: 100 (no change from production)
PWA: 60 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@SoonIter SoonIter requested review from chenjiahan and 9aoy January 2, 2025 08:58
(originSource) =>
originSource.replace(
'var fullhref = __webpack_require__.p + href;',
'var fullhref = __webpack_require__.rsbuildLoadStyleSheet ? __webpack_require__.rsbuildLoadStyleSheet(href, chunkId) : (__webpack_require__.p + href);',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be an unsafe replacement, if the source code has changed, this replacement may not work or may break the original code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no better solution at present, it is a part of ancient runtime code from mini-css,
if changed in rspack, eco-ci would take effect for this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JSerFeng cc, can you offer some advice?

Copy link
Member Author

@SoonIter SoonIter Jan 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feature relies on css-extract, so it meets expectations and will have a higher dependence on css-extract, even if I don't use the replace method, any changes to css-extract runtime will have an influence to assets-retry

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, let's keep track of the impact of runtime code changes through our ecosystem-ci.

@chenjiahan chenjiahan merged commit 3fd33a4 into main Jan 5, 2025
10 checks passed
@chenjiahan chenjiahan deleted the miniCssF branch January 5, 2025 09:07
@chenjiahan chenjiahan mentioned this pull request Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: [plugin-assets-retry] retry asyncChunk css file, addQuery not working
2 participants