search

inveniosoftware / rfcs

RFCs for Invenio.
https://rfcs.readthedocs.io
4 stars 15 forks source link

[Proposal] use rspack, pnpm to improve assets build time #85

Open utnapischtim opened 2 months ago

utnapischtim commented 2 months ago

Motivation

Improve the time a invenio-cli assets build needs to build the assets.

Summary

PR's

open problems

pdfjs

the invenio-previewer PR is not only necessary to fix bugs in the pdf viewer but also necessary because the current used version of pdfjs couldn't be build with rspack. Further upgrading to the newest version fixes the build error but doesn't produce a asset which works in the browser. (without testing i hope that this PR fixes both problems)

not yet a PR because it needs some discussion npm -> pnpm, webpack -> rspack

how could we move from npm to pnpm? https://github.com/inveniosoftware/pynpm/blob/1d16a60df39ca5ba23feeca9e3142be35cf69c92/pynpm/package.py#L31 chaning the default value for npm_bin seems the easiest solution but could irritate users of that package. the only other option is to give it to the construction: https://github.com/inveniosoftware/pywebpack/blob/fcb464a46bb88cbb7731689e9869354536fe4df8/pywebpack/project.py#L48 which is also not that easy, because we are in the webpack configuration, but we are moving from webpack to rspack. so should we only change from webpack to rspack and keep the whole pywebpack package, or should we implement that completely new to have the wording correct?

NOTE: pnpm install needs the --shamefully-hoist parameter otherwise the peer dependencies are not found by rspack

test out the configuration for invenio-cli assets watch

not yet tested

with_appcontext

most of the code copied over to the invenio-cli PR has been copied because of the with_appcontext problem. the code in the various packages are based on current_app which needs the app context. this has to be solved somehow to make the rewrite in invenio-cli possible

possible solutions (needs more thinking)

rename Webpack classes to Asset classes move pynpm and pywebpack functionality to invenio-assets and rename the class to the problem they solve without the tool in its name. reason to move: the functionality is used exclusively there (to be proved) add with_appcontext decorator to the invenio-cli assets build cli command to not have to refactor a lot of code

NOTE: replacing npm with pnpm is still a little bit difficult if pynpm should not be changed? the problem is the parameter --shamefully-hoist which is necessary that pnpm install works

big problem in my eyes at the moment

flask has to be installed by invenio-cli which is not the case at the moment!

max-moser commented 1 month ago

For the PDF.js upgrade in Invenio-Previewer, there's a handful of pull requests that implement @ntarocco 's feedback on the original PDF.js upgrade PR: