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

Hoist href urls construction logic to localVars in runtime #521

Open
ernestostifano opened this issue Apr 1, 2020 · 8 comments
Open

Hoist href urls construction logic to localVars in runtime #521

ernestostifano opened this issue Apr 1, 2020 · 8 comments

Comments

@ernestostifano
Copy link

  • Operating System: macOS Catalina (10.15.3)
  • Node Version: 13.12.0
  • NPM Version: 6.14.4
  • webpack Version: 4.42.1
  • mini-css-extract-plugin Version: 0.9.0

Expected Behavior / Situation

See Modification Proposal below.

Actual Behavior / Situation

Currently, the runtime href urls construction logic is placed inside the requireEnsure function (using compilation.mainTemplate.hooks.requireEnsure hook)

Differing from Webpack's on-demand JS loading implementation. Which adds jsonpScriptSrc function to localVars for constructing script src urls.

Actual situation makes it impossible for other plugins to reuse and/or extend MiniCssExtractPlugin runtime functionality.

// mini-css-extract-plugin CSS loading
var cssChunks = {"0":1,"2":1,"6":1,"7":1,"8":1,"9":1,"10":1,"11":1,"12":1};
if(installedCssChunks[chunkId]) promises.push(installedCssChunks[chunkId]);
else if(installedCssChunks[chunkId] !== 0 && cssChunks[chunkId]) {
    promises.push(installedCssChunks[chunkId] = new Promise(function(resolve, reject) {
        var href = "static/css/" + ({"0":"app~06837ae4","3":"boot~31ecd969","4":"react~1f20a385","5":"modules~253ae210"}[chunkId]||chunkId) + "." + {"0":"b100e6da","2":"fa9261ae","3":"31d6cfe0","4":"31d6cfe0","5":"31d6cfe0","6":"fa9261ae","7":"fa9261ae","8":"fa9261ae","9":"fa9261ae","10":"fa9261ae","11":"fa9261ae","12":"fa9261ae"}[chunkId] + ".chunk.css";
        var fullhref = __webpack_require__.p + href;

        /* REST OF THE CODE */

    })

For example, a custom loading algorithm, that may add AJAX capabilities or events support can access requested JS chunks paths by calling jsonpScriptSrc(), but not requested CSS paths.

Modification Proposal

compilation.mainTemplate.hooks.localVars:

// link path function
function jsonpLinkHref(chunkId) {
    return !{"0":1,"2":1,"6":1,"7":1,"8":1,"9":1,"10":1,"11":1,"12":1}[chunkId] ? null : __webpack_require__.p + "static/css/" + ({"0":"app~06837ae4","3":"boot~31ecd969","4":"react~1f20a385","5":"modules~253ae210"}[chunkId]||chunkId) + "." + {"0":"b100e6da","2":"fa9261ae","3":"31d6cfe0","4":"31d6cfe0","5":"31d6cfe0","6":"fa9261ae","7":"fa9261ae","8":"fa9261ae","9":"fa9261ae","10":"fa9261ae","11":"fa9261ae","12":"fa9261ae"}[chunkId] + ".chunk.css";
}

compilation.mainTemplate.hooks.requireEnsure:

// mini-css-extract-plugin CSS loading
var fullhref = jsonpLinkHref(chunkId);
if(installedCssChunks[chunkId]) promises.push(installedCssChunks[chunkId]);
else if(installedCssChunks[chunkId] !== 0 && fullhref) {
    promises.push(installedCssChunks[chunkId] = new Promise(function(resolve, reject) {
        var href = fullhref.substr(1);

        /* REST OF THE CODE */

    })

This simple modification solves everything and matches Webpack's JS on-demand loading logic.

Now, mini-css-extract-plugin CSS loading snippet could be also moved up and down by other plugins inside requireEnsure function and even a pre-processed path could be passed to it (EX: A Blob object url).

Requested CSS path is now available throughout requireEnsure by calling jsonpLinkHref()

Note: I am ready to immediately submit a PR for this. Change would be very simple and not breaking + No extra size or cpu overhead. Please let me know.

Additional Note: I think this modification is already a good improvement. But if customization shall be increased, new Webpack 5 hooks implementation could be used for exposing custom internal hooks. (I would consider this separately)

@alexander-akait
Copy link
Member

Can you provide real use case?

@alexander-akait
Copy link
Member

@ernestostifano friendly ping

@ernestostifano
Copy link
Author

ernestostifano commented Nov 6, 2020

@evilebottnawi sorry for the delay, I think I forgot about this issue (thanks for the ping).

As an example of what I was trying to say, I can point you to this specific file of a plugin I was working on a while ago (still work in progress). It extends original Webpack JSONP loading logic with an event interface, allowing users to handle resources loading (or not) when needed (if event is not handled by user, Webpack will handle it normally). Same thing cannot be done with mini-css-extract-plugin runtime code for the reasons explained above. It would have been nice to manage both cases through a single event interface.

This idea is part of a bigger project I am working on that will allow users to get deeper control on resources loading/caching/management, including static files size injection (both compressed and not) during build time for latter progress calculation without the need of additional HEAD requests.

I've also seen you participating in some Webpack issues/PRs regarding JSONP replacement with XHR (see this, this and this) and even though I'm not a huge fan, it could be another valid reason to make mini-css-extract-plugin runtime logic more reusable and similar to JS files handling logic. (By the way, my approach is to load resources via XHR and then pass a Blob URL to the script tag created by Webpack to avoid many of the issues people is facing with XHR+eval).

For my specific use case, I decided to fully replace mini-css-extract-plugin runtime code with a modified version, but it is not an elegant solution because it involves copying a lot of non related functionalities from your runtime module.

@alexander-akait
Copy link
Member

I would like to understand how special (rare case), perhaps in your case, creating your own plugin is the best way, unfortunately we cannot cover all 100% cases, I hope you understand this

@ernestostifano
Copy link
Author

@evilebottnawi I absolutely understand that. But generally speaking, isn't it better to make plugins more flexible when possible? I still think that the proposed modification does not affect current behavior and should not break anything.

Why does Webpack expose localVars hook if not for this?

I really like mini-css-extract-plugin and I was trying to add support for it in the best way possible.

However, if you think that this proposal is not viable, I will continue to use the modified version of the code and we can close this issue.

@alexander-akait
Copy link
Member

I absolutely understand that. But generally speaking, isn't it better to make plugins more flexible when possible?

Yep, we try to do it

Why does Webpack expose localVars hook if not for this?

We have some limitation for mini-css-extract-plugin

I really like mini-css-extract-plugin and I was trying to add support for it in the best way possible.

maybe you can provide link on your modified version or send a PR with example

I want to investigate it deeply

@ernestostifano
Copy link
Author

@evilebottnawi I will check the latest version of your runtime module and submit a PR.

@alexander-akait
Copy link
Member

Big thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants