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

Implement alternative WebpackBundleProject for rspack #176

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

max-moser
Copy link
Contributor

@max-moser max-moser commented Feb 20, 2025

  • Enables the rspack-based build by setting the (already existing) config WEBPACKEXT_PROJECT = "invenio_assets.webpack:rspack_project"
  • Enables pnpm builds by setting the (new) config ASSETS_NPM_PKG_CLS = "pynpm:PNPMPackage"

Basically this PR provides adjusted versions of package.json and webpack.config.js for rspack: rspack-package.json and rspack.config.js.
The idea here is basically the rule of three, as popularized by Martin Fenner Fowler.
Right now we have two options (webpack and rspack) – once we get more than that, we should actually start refactoring.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorted the devDependencies

"@swc/helpers": "^0.5.11",
"sass-loader": "^16.0.0",
"swc-loader": "^0.2.6",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

separated the rspack-specific deps with an empty line

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further options for thinning-down this file:

  • Since we depend on @inveniosoftware/esling-config-invenio, we can probably remove all the dev dependencies that are already defined there
  • Because we're using swc, all @babel/* packages can also probably go

Comment on lines 72 to 78
//preset: 'verbose',
// warnings: true,
// errors: true,
// errorsCount: true,
// errorStack: true,
// errorDetails: true,
// children: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some parts here could take some more cleanup

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these options should only be picked up by pnpm; from my experience there was no negative effect on npm runs

@@ -59,7 +59,7 @@ def init_config(self, app):
)

# Flask-WebpackExt config
app.config.setdefault("WEBPACKEXT_PROJECT", "invenio_assets.webpack:project")
app.config.setdefault("WEBPACKEXT_PROJECT", "invenio_assets.webpack:webpack_project")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the original value is still provided for backwards compatibility, but moving forward i'd suggest going with the new fallback value

"css-loader": "^6.0.0",
"css-minimizer-webpack-plugin": "^4.2.0",
"eslint-config-react-app": "^7.0.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this guy caused some issues with the pnpm + webpack build (but not for npm + webpack) due to some apparent conflicts in .eslintrc configs [1] – the simplest "fix" was to remove this dependency... not sure if that's the best way to go, but it did make the builds run through successfully

[1] unfortunately i don't have the error logs at hand anymore, but they should pop up again if this dep gets added back

@max-moser max-moser force-pushed the mm/rspack branch 2 times, most recently from c103a5e to 5e2074f Compare February 26, 2025 14:39
* rspack provides a "--mode" flag which replaces "NODE_ENV"
* rspack does not support the "--progress" flag
* "eslint-config-react-app" caused PNPM-based builds to complain about
  conflicting eslint configs
* duplicates from @inveniosoftware-eslint-config-invenio
* they still got installed even after removing them from the
  devDependencies here, so they are pulled in from somewhere else
  already
* checked separately for the webpack and rspack builds
@@ -162,7 +162,7 @@ var webpackConfig = {
},
// Inline images smaller than 10k
{
test: /\.(png|jpe?g|gif|svg)(\?.*)?$/,
test: /\.(avif|webp|png|jpe?g|gif|svg)(\?.*)?$/,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added two new image file formats that we've been using at TUW for a while already, same for rspack

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants