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

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

Open
utnapischtim opened this issue Sep 12, 2024 · 1 comment
Open

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

utnapischtim opened this issue Sep 12, 2024 · 1 comment
Labels
Proposal: Pending Proposal for new RFC, pending triage

Comments

@utnapischtim
Copy link

utnapischtim commented Sep 12, 2024

Motivation

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

Summary

  • create a create_cli app as small as possible to load as fast as possible
  • move from webpack to rspack to improve assets build time from around >50sec to <10sec
  • move from npm to pnpm to improve installation time from >60sec to <11sec
  • combined improvement from around 3 minutes to 22seconds

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!

@utnapischtim utnapischtim added the Proposal: Pending Proposal for new RFC, pending triage label Sep 12, 2024
@max-moser
Copy link

max-moser commented Oct 1, 2024

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:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal: Pending Proposal for new RFC, pending triage
Projects
None yet
Development

No branches or pull requests

6 participants