Skip to content

feat: faster babel-loader by excluding node_module #644

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: faster babel-loader by excluding node_module #644

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion config/webpack.dev-stage.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ module.exports = merge(commonConfig, {
// Babel is configured with the .babelrc file at the root of the project.
{
test: /\.(js|jsx)$/,
exclude: /node_modules\/(?!@(open)?edx)/,
exclude: /node_modules/,
use: {
loader: 'babel-loader',
options: {
Expand Down
2 changes: 1 addition & 1 deletion config/webpack.dev.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ module.exports = merge(commonConfig, {
// Babel is configured with the .babelrc file at the root of the project.
{
test: /\.(js|jsx|ts|tsx)$/,
exclude: /node_modules\/(?!@(open)?edx)/,
exclude: /node_modules/,
Copy link
Member

Choose a reason for hiding this comment

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

Which means that only @openedx and @edx assets were excluded.

The current configuration here is excluding all node_modules except assets scoped with @openedx or @edx using a negative lookup (?!), which I believe is a bit different than what's described above and in the PR description.

Historically, the @openedx and @edx scoped packages were not transpiled on their own and we relied on @openedx/frontend-build to do the transpilation of the @openedx and @edx scoped packages, but exclude all other node_modules.

This change begins excluding all node_modules, including the @openedx and @edx scoped packages.

For this change, we'd likely need to confirm that all @openedx and @edx scoped packages are already adequately transpiled upstream without relying on this negative lookup in the exclude in the Webpack builds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oooooh my bad, I had definitely misread that regular expression. Still, the build time improvements are worth it, if the openedx/edx assets are correctly transpiled.

Copy link
Contributor

@bradenmacdonald bradenmacdonald Apr 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

frontend-app-authoring definitely relies on installing un-transpiled plugins and assumes that frontend-build will transpile them because they're in the @openedx-plugins/* namespace, or something like that. We could change this somewhow, but there may be other examples of this expectation. ref

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should transpile only jsx/tsx files from node_modules/@(open)?edx, and not all js files?

Copy link
Member

Choose a reason for hiding this comment

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

Is the rationale for only transpiling jsx/tsx files that only JSX syntax needs to be transpiled? I suppose I'd also think about other cases like newer JS syntax used in a project which might also require transpiling for older browsers, irrespective of JSX syntax itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the rationale for only transpiling jsx/tsx files that only JSX syntax needs to be transpiled?

Yes.

I suppose I'd also think about other cases like newer JS syntax used in a project which might also require transpiling for older browsers, irrespective of JSX syntax itself.

I don't know, I'm really just following recommendations that I'm seeing everywhere. My suggestion would be the opposite: to package openedx packages like everybody else does, without jsx/tsx/ts artifacts, and only with compiled assets. I don't see any reason why we should be doing things differently.

Copy link
Member

Choose a reason for hiding this comment

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

My suggestion would be the opposite: to package openedx packages like everybody else does, without jsx/tsx/ts artifacts, and only with compiled assets. I don't see any reason why we should be doing things differently.

Yeah, I totally agree this is the direction Open edX should strive for (i.e., relying on best practices as much as possible). We would just need to coordinate to be sure all @openedx and @edx scoped packages are adequately transpiled to our liking before we can change this behavior in frontend-build so it's not considered "breaking", if at all possible, and/or give adequate heads up to the community regarding the change if action is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless I'm mistaken, the build will simply fail when we attempt to load jsx/tsx files without a transpiler, because of syntax errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion would be the opposite: to package openedx packages like everybody else does, without jsx/tsx/ts artifacts, and only with compiled assets. I don't see any reason why we should be doing things differently.

There are some reasons to publish things that aren't transpiled:

(1) it means that the MFE decides what babel transforms are necessary, rather than the library. For example, our transpiled version of frontend-component-header currently published on NPM has these babel compatibility shims in every .js file; the example below (in Header.js) adds 1KB and is completely pointless. (This is due to a bad configuration within frontend-component-header's babel config, but the point is that it should be the MFE's babel config that controls the presence of these shims, not the library.) Once these shims are in the library they'll always be in the MFE build; there's no way to exclude them later.

Screenshot

(2) it makes the process of publishing libraries much simpler. No babel, no webpack, no build process at all.

That said, in light of the performance concerns you're raising, I think it does still make sense to transpile things before publishing. I'm just pointing out that it's also considered acceptable not to do it, and sometimes preferred.

For what it's worth, my personal preference is to publish to JSR instead of NPM. Publishing there is way easier, as you just publish raw TypeScript/TSX files, and the registry automatically serves the transpiled version if your runtime (e.g. Node) requests it, or the raw source if your developer environment (e.g. VS Code) or runtime (e.g. Deno) supports it. It's also fully backwards compatible with NPM.

use: {
loader: 'babel-loader',
options: {
Expand Down
2 changes: 1 addition & 1 deletion config/webpack.prod.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ module.exports = merge(commonConfig, {
// Babel is configured with the .babelrc file at the root of the project.
{
test: /\.(js|jsx|ts|tsx)$/,
exclude: /node_modules\/(?!@(open)?edx)/,
exclude: /node_modules/,
use: {
loader: 'babel-loader',
options: {
Expand Down