-
-
Notifications
You must be signed in to change notification settings - Fork 9.1k
feat: Removing the need for async Import for #18812 #18942
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
base: main
Are you sure you want to change the base?
Conversation
For maintainers only:
|
@Neerajpathak07 tests are failed |
Is there any documentation related to the tests that I can refer to. |
@Neerajpathak07 Testa are failed, you need to fix them |
@ScriptedAlchemy I think the code in this project should definitely be divided into several parts because it's literally copy-paste with a lot of unnecessary/not tested things... What exactly do we need to do to solve the original problem? |
The idea behind it is you do not need a import() to make federation work. No eager consumption issue is possible because the entrypoints use async startup and call ensureChunkHandler on themselves to load eager shares or remotes before evaluation of the entrypoint module require() Very similar to async entrypoint startup We call require.f.consumes and remotes, the promise All and finally require(entrymodule) Now user code does not need modification because we do not depend on the mechanics of require ensure to ensure the async parts. the entrypoint basically ensures itself |
@ScriptedAlchemy I understand what you are trying to solve, but I am talking about the current pr, it looks like a not very well written implementation and no tests, so maybe we should break it into pieces |
Agreed, but i think that if we do so, we will have to accept that some aspects may be untestable on their own. We may have to queue some tests till there is "enough" parts available to actually test no import() itself. What do you think? |
@ScriptedAlchemy I understand this, we can use mocks or even unit testing temporarily somewhere to avoid regressions |
Perfect, unit test will work. ill look through this pr and my own source code and see how i can break it tup This was a massive implementation on my end so i understand haha. I think i might have a branch somehwere that implemented it partially, since i was not planning to support webpack and was going to go right to rspack, so i did POC about half of it in webpack branch just to provide reference of change to rspack team. Ill revive it and see if i can assist in breaking it up. What are your thoughts about refactoring the ESM entry generation so that theres a central startuphelper file for rendering the entry startup, cjs and web use generateEntryStartup, but esm is its own file with no util function - would be nice to consolidate it into one file for startup helper string generation instead (this pr copy pastes that esm entry generation as you can see) |
I'm not against it, the only thing we have to do is implement hooks to insert this behavior, doing it via inline (like - https://github.com/webpack/webpack/pull/18942/files#diff-1f6c9dd2a670408f845477b54bae6d20edd21b5304a7bfc8313eb904f844e4b3R108 and https://github.com/webpack/webpack/pull/18942/files#diff-1f6c9dd2a670408f845477b54bae6d20edd21b5304a7bfc8313eb904f844e4b3R43) is not a very supported architecture |
Agreed. Alex would you be willing to help me adjust some of the aspects here? If I break it up into small parts. Just for sake of speeder implementation it would be useful to have someone faster and more knowledge in day to day of webpack. We can discuss ad-hoc and I'll start with breaking it up and implement the better parts first then we work down to the more difficult parts |
@ScriptedAlchemy Do you mean this pull requests? First let's use hooks and move logic to MF plugins |
What hooks and logic Alex? Do you have pseudo example of what you're imagining? |
@ScriptedAlchemy For example - https://github.com/webpack/webpack/pull/18942/files#diff-1f6c9dd2a670408f845477b54bae6d20edd21b5304a7bfc8313eb904f844e4b3R43 and https://github.com/webpack/webpack/pull/18942/files#diff-1f6c9dd2a670408f845477b54bae6d20edd21b5304a7bfc8313eb904f844e4b3R108, why do not create a plugin or put in the exsting inside https://github.com/webpack/webpack/tree/main/lib/container? The current file is definitely not quite correct place. |
I ask here to simply put everything in the right places and hook, and hook only when required, the current code lies in unnecessary places and is included even when the federation is not used |
ahh i see what you mean, basically lets make a new plugin to consolidate various hooks we are calling instead of adding it into another existing plugin, try to isolate it into a specific plugin for the cause. Okay ill get to it some time next week and start sending small PRs, maybe we create a staging branch so we do not merge to main till we collect enough approved parts in the staging branch? Either way, ill try and start some of this work next week or week after |
We can, let's do it when you send a Pull request 👍 |
Perfect. I'll start once I return from conferences some time next week |
Fixes:-#18812
What kind of change does this PR introduce?
This pr adds a logic to remove the need for async import in
StartupHelpers.js
andStartupChunkDependenciesPlugin.js
.Additionally referencing all the plugins to
Runtimeglobals.js
.Did you add tests for your changes?
yes
Does this PR introduce a breaking change?
No
What needs to be documented once your changes are merged?
No