-
Notifications
You must be signed in to change notification settings - Fork 45
Enable PNPM (rspack is part of Invenio-Assets) #385
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
Enable PNPM (rspack is part of Invenio-Assets) #385
Conversation
c6ff314 to
187dfa2
Compare
| response = run_interactive( | ||
| op, env={"PIPENV_VERBOSITY": "-1"}, log_file=log_file | ||
| op, | ||
| env={"PIPENV_VERBOSITY": "-1", **js_pkg_man.env_overrides()}, |
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.
here we tell invenio about the JS package manager that we want to use, via environment variables
| ops = [py_pkg_man.run_command("invenio", "collect", "--verbose")] | ||
|
|
||
| if force: | ||
| ops.append(pkg_man.run_command("invenio", "webpack", "clean", "create")) | ||
| ops.append(pkg_man.run_command("invenio", "webpack", "install")) | ||
| ops.append(py_pkg_man.run_command("invenio", "webpack", "clean", "create")) | ||
| ops.append(py_pkg_man.run_command("invenio", "webpack", "install")) | ||
| else: | ||
| ops.append(pkg_man.run_command("invenio", "webpack", "create")) | ||
| ops.append(py_pkg_man.run_command("invenio", "webpack", "create")) | ||
| ops.append(self._statics) | ||
| ops.append(pkg_man.run_command("invenio", "webpack", "build")) | ||
| ops.append(py_pkg_man.run_command("invenio", "webpack", "build")) |
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.
there is still a good amount of optimization potential by calling the invenio command only once and thus saving all the redundant application initializations which eat up a lot of time.
one way to move forward is to implement a single larger command (e.g. in invenio-app-rdm) that performs all the steps currently implemented here, based on the provided CLI args.
@ntarocco has pointed out to me that something very much like that has already been done in Invenio-App-ILS
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.
62a7f3a to
0ee5d67
Compare
| AssetsFunction(_prelink_dist, "Executing prelink-dist script..."), | ||
| AssetsFunction(_link_package_single_step, "Linking module to assets..."), | ||
| AssetsFunction(_postlink_dist, "Executing postlink-dist script..."), |
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.
3 is better than 1!
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.
as discussed, we're basically executing the same scripts with pnpm as we did with npm, except for the part where the actual linking happens (which is more sane with pnpm.
since we don't have preprelink-dist, postprelink-dist, prepostlink-dist and postpostlink-dist scripts defined in our packages i think we should be executing the same set of scripts as before here.
| # This is geared towards Invenio JS packages... | ||
| # But so are all the other steps | ||
| status_code = assets_pkg.link( | ||
| str(module_pkg.package_json_path.parent / "dist") |
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.
the fact that we're linking against the "dist" directory and not using the scripts requires some knowledge about the target package... but so does executing the link-dist script, which is not exactly a standard AFAIK 😅
| class AssetsFunction: | ||
| """Function to run an assets command with `assets_pkg` and `module_pkg`.""" | ||
|
|
||
| function: Callable[[NPMPackage, NPMPackage], ProcessResponse] |
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.
the dataclass wants to have some type annotation, but Callable should do the trick already.
i have no strong attachment to this type hint otherwise.
0ee5d67 to
64964fe
Compare
* bump pynpm dependency for the PNPMPackage * tell invenio about which JS package manager is actually selected currently (npm or pnpm), via the new `WEBPACKEXT_NPM_PKG_CLS` config * also, use the module_pkg to execute the install command in the target path (rather than the instance's path, which made a difference in the case of pnpm)
* as discussed, we implement the `pnpm link` to basically do the same thing as the current `npm link` procedure, to not require any changes in JS packages
64964fe to
0a695af
Compare
|
Once again (like with the PRs for Flask-WebpackExt & Invenio-Assets), we've discussed this feature and its implementation during the InvenioRDM community workshop 2025 etc. so the lack of official approval shouldn't indicate my rebellion here. |
Information (updated 2025-03-28)
This PR is the frontend cousin of #384 (which similarly enabled
uvfor python).Enabling
pnpmNote: This feature requires⚠️
flask-webpackextv2.1.0, and a recent (global) installation of the CLI commandpnpm(10+)Similar to
uv,pnpmcan be enabled in.invenio(which is the "public" configuration file that is intended to be shared between developers via version control).Unlike
uv, there are (currently [1]) no artifacts in the project directory that would indicate whethernpmorpnpmshould be used and thus the fallback is simplynpm(i.e.pnpmhas to be explicitly configured).Configuration in
.invenio:[1] this may change once the "reusable frontend lock file" logic gets merged as:
package.lock->npmpnpm-lock.yaml->pnpmEnabling
rspackNote: This feature requires⚠️
invenio-assetsv4.1.0 (or v3.2.0 for InvenioRDM v12)invenio.cfg:Like any other configuration item, it could alternatively be specified as an env var with prefix (
INVENIO_WEBPACKEXT_PROJECT="invenio_assets.webpack:rspack_project"), but this is out of scope for here – iykyk.Why aren't
pnpmandrspackboth configured ininvenio.cfg?While it may seem strange that
rspackuses a different configuration mechanism (invenio.cfg) thanpnpm(.invenio), the reason here is simple:WEBPACKEXT_PROJECT = "invenio_assets.webpack:project") that could for instance be configured viainvenio.cfg.pnpmswitch did not exist previously, and bothinvenio-clias well asinvenio(the application) have to agree on which JS package manager is used (andinvenio-clidoes not parseinvenio.cfgitself).What's with
react-invenio-formsandpnpm?The
link-distscript in Invenio-y JS packages is geared towardsnpm linkand thus obviously won't work nicely forpnpm.(Also, requiring these nonstandard script entries means that only these Invenio-y JS packages are compatible.)
This should likely be reworked in these JS packages, but that's a bit too much effort for now.
For now, we've decided to simply write the
pnpmlogic to perform basically the same steps asnpmwould, except for the actual linking step (which only requires a single step withpnpm).For those interested, I recommend searching the comments mentioning
prepostlink-distand others.More context (for historical reasons)
Second part of #383 – support for
pnpmandrspackThis PR won't work as standalone, it still requires changes to core Invenio packages.
Either inveniosoftware/invenio-assets#176
Or inveniosoftware/invenio-assets#172, inveniosoftware/flask-webpackext#26, inveniosoftware/pywebpack#47
Depending on the choice, some tweaks need to be performed in this PR here.
Also,npmis still hard-coded in React-Invenio-Forms which needs to be addressed.Edit: This has been resolved, see above.