feat: faster babel-loader by excluding node_module

[regisb]

The role of babel-loader is twofold:

• Compile jsx (react) assets to js
• Make js assets compatible with older browsers

See the official documentation here: https://webpack.js.org/loaders/babel-loader/

It is widely recommended to exclude files in node_modules/ from that loader. See for instance: https://webpack.js.org/loaders/babel-loader/#babel-loader-is-slow Indeed, such files are assumed to be already compatible with older browsers, and built from jsx.

However, the base configuration in frontend-build had the following exclude directive:

exclude: /node_modules\/(?!@(open)?edx)/,

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

With this change, we reduce the build time significantly for all MFEs. For instance:

• frontend-app-profile: 86s -> 64s (-26%)
• frontend-app-learning: 167s -> 110s (-34%)

We observe a change in the fingerprint of app.js and app.css, but we don't consider these changes to be significant. The generated MFEs still worked when we tested them. If we realize later that some modules need to be compiled with babel, then we should explicitly include them with and directives, as documented here: https://webpack.js.org/loaders/babel-loader/#some-files-in-my-node_modules-are-not-transpiled-for-ie-11

[openedx-webhooks]

Thanks for the pull request, @regisb!

This repository is currently maintained by @openedx/committers-frontend-build.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

• If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
  • This process (including the steps you'll need to take) is documented here.
• If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

• Dependencies

  This PR must be merged before / after / at the same time as ...

• Blockers

  This PR is waiting for OEP-1234 to be accepted.

• Timeline information

  This PR must be merged by XX date because ...

• Partner information

  This is for a course on edx.org.

• Supporting documentation
• Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

---

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

• The size and impact of the changes that it introduces
• The need for product review
• Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

[regisb]

This is ready for review.

[adamstankiewicz]

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.

[regisb]

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.

[bradenmacdonald]

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

[regisb]

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

[adamstankiewicz]

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.

[regisb]

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.

[adamstankiewicz]

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.

[regisb]

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

[bradenmacdonald]

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.

(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.

[regisb]

FTR frontend-app-ora-gradingis doing some funky rewrite of the exclude rule: https://github.com/openedx/frontend-app-ora-grading/blob/9348c4bb4c36937981d8b60e6ef72b36f8c65ac1/webpack.prod.config.js#L11

config.module.rules[0].exclude = /node_modules\/(?!(query-string|split-on-first|strict-uri-encode|@edx))/;

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (3c80898) to head (4e6ea3a).
Report is 1 commits behind head on master.

Additional details and impacted files

@@      Coverage Diff      @@
##   master   #644   +/-   ##
=============================
=============================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.