application-manager: use Webpack v5#9451
Conversation
|
cc @eclipse-theia/core |
vince-fugnitto
left a comment
There was a problem hiding this comment.
I verified the following for both browser and electron targets:
- icons are loaded successfully.
- fonts are loaded successfully.
- syntax-highlighting works correctly.
I also verified that the updated dependencies are license compatible (through clearly defined).
I'll let other review as well 👍
|
LGTM, I can confirm that everything works as expected in the Furthermore, this change reduces the size of the webpack bundles by a few hundred KB and the |
c62e4af to
2b78899
Compare
|
Thanks for the reviews. Since the reference to |
Bump Webpack from 4 to 5 and update the generated configurations to use the new APIs. Some packages are no longer required as Webpack provides similar features now, see https://webpack.js.org/guides/asset-modules/. On the other hand, Webpack doesn't provide some of the shims it used to. We need to setup some of those ourselves. The frontend entrypoint now requires `setimmediate` to shim the `setImmediate` APIs, and we use Webpack's `ProvidePlugin` to expose Node's `Buffer` API for our dependencies to work.
2b78899 to
349aee0
Compare
| "setimmediate": "^1.0.5", | ||
| "source-map-loader": "^2.0.1", | ||
| "source-map-support": "^0.5.19", | ||
| "style-loader": "^2.0.0", |
There was a problem hiding this comment.
@paul-marechal One thing I still don't understand here is why these are not in the devDependencies section. Does webpack not take care of including them in the mangled output?
There was a problem hiding this comment.
devDependencies are not installed when a package is depended upon meaning those packages would be missing for dependents. Hence those being regular dependencies.
Regarding the bundling, setimmediate is required in src-gen and bundled. The loaders should never reach the final bundles, although they need to be in the environment used to run Webpack (using Theia CLI.)
There was a problem hiding this comment.
Thinking about it, if @theia/application-manager only appears as a dev-dependency in a application package.json it should keep working indeed. But from the POV of @theia/application-manager these dependencies must be pulled alongside it, so they can't be declared as devDependencies.
There was a problem hiding this comment.
Ok, so the correct way to consume all that is to have @theia/application-manager as a dev dependency in the application package (like examples/browser)?
There was a problem hiding this comment.
Experimenting a bit further with this: unless I add an explicit dependency in my application to stuff like circular-dependency-plugin only gets installed into node_modules/@theia/application-manager/node_modules/circular-dependency-plugin and thus is not available for require(...)from gen-weback-config.js. So I think we need to fix https://theia-ide.org/docs/composing_applications.
There was a problem hiding this comment.
What you describe sounds like a hoisting problem and it can be painful to troubleshoot. It is part of why I wanted to limit implicit dependencies as much as possible. In this specific case I am not sure how you could dedup the package. yarn why can be useful to see all dependents, and if every package depends on the same version yarn should install it only once at the root of your node_modules.
There was a problem hiding this comment.
How do you folks build your application packages? Do you consume the theia packages as npm modules?
There was a problem hiding this comment.
Do you consume the theia packages as npm modules?
Most of the time yes.
Bump Webpack from 4 to 5 and update the generated configurations to use
the new APIs.
Closes #9435
How to test
Only impacts the browser/electron-browser, everything should work like before.
Review checklist
Reminder for reviewers