-
Notifications
You must be signed in to change notification settings - Fork 69
chore: reorganize prod dependencies #787
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
base: master
Are you sure you want to change the base?
Conversation
Thanks for the pull request, @regisb! This repository is currently maintained by 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 approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo 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:
🔘 Get a green buildIf 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: 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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
This is to allow `npm install --omit=dev` across all MFE apps.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #787 +/- ##
=======================================
Coverage 86.71% 86.71%
=======================================
Files 48 48
Lines 1393 1393
Branches 293 293
=======================================
Hits 1208 1208
Misses 172 172
Partials 13 13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I think I need more context to understand why If this PR landed as it stands now it would definitely break things. I'd like to understand the intent before trying to figure out a path forward for this that won't break things. |
@brian-smith-tcril What will this break? If it were going the other way I can see it breaking stuff, but how would moving devDeps into deps break anything?
|
Because we currently support MFEs using multiple versions of some of the dependencies being moved via peer deps. Specifically Paragon 22 and 23, and React 17 and 18. |
@brian-smith-tcril Then shouldn't we be specifying the dependency here as e.g. Paragon |
@bradenmacdonald by specifying the version ranges in I know it is possible to set version ranges that span multiple major versions in direct This brings me back to my original question. Why/where is Of the packages this PR is moving from "@openedx/frontend-build": "^14.3.0",
"@openedx/paragon": "^23.3.0",
"react": "18.3.1",
"react-dom": "18.3.1",
"react-redux": "^8.1.1",
"react-router-dom": "^6.6.1",
"redux": "4.2.1", already exist as The other 2 being moved are "@edx/brand": "npm:@openedx/brand-openedx@^1.2.2",
"@edx/browserslist-config": "1.5.0", If those are the problematic ones then I could see either adding them as |
I was completely ignorant of peerDependencies before reading your comment, so thanks for taking the time to explain :) Yet, your explanations (or the ones I found here) do not match my experience:
To reproduce, run:
I'm using:
My reasoning when I opened this PR was:
It's quite possible that I went overboard with step 3, in which case I'm ready to amend this PR. |
@regisb why are you running |
I believe I was testing frontend-platform, because I figured that I would have to fix frontend-platform's devDependencies before I could work on MFE apps. Isn't that a valid test case? It looks like frontend-build is correctly installed in MFE apps, when they depend on frontend-platform. When I create a package.json with:
Then I install dependencies with |
It is not. The The
When running |
👌 I'll update this PR accordingly. In the same spirit, should |
The |
Additionally, one related item worth explicitly mentioning here is that some dependencies might only strictly support the Other library repos have a standalone
@regisb @brian-smith-tcril My understanding is the build tools such as Babel (used within frontend-platform's build, source) take into account any configured
@bradenmacdonald I'm not sure I follow the recommendation for frontend-build to be a regular Webpack documentation recommends installing Webpack with
|
@adamstankiewicz I think @regisb's the overall goal here is to be able to run This goes against my working understanding of direct As for |
I'm learning a lot from your conversation -- I'll just take a back seat and implement whatever decision you make. For instance, I could open PRs to move browserlists-config to the prod dependencies of MFE apps. |
I know we had quite a few conversations around this when openedx/open-edx-proposals#615 was being discussed. I think when I approved it I was focused on the "make sure things that shouldn't be direct deps are dev deps" aspect of it, and not fully grokking the "some things that are currently dev deps should move to be direct deps" aspect of it. One thing that I don't see mentioned on that PR is impact on bundle size. I know keeping something out of direct deps is a pretty surefire way to keep it out of the bundle, but I know it's far from the only way to do so. |
I can't answer with confidence on that. As I explained here, I'm not too sure how to test the changes in dependencies locally. |
@brian-smith-tcril Yes, I agree and have the same understanding as you regarding direct
@brian-smith-tcril This indeed seems like a problem, and an unintended consequence from introducing support for On some initial investigation, I'm wondering if this concern could be mitigated by adding the following to frontend-platform's {
"peerDependenciesMeta": {
"@openedx/frontend-build": {
"optional": true
}
}
}
|
That's exactly why I want us to split
Let's move those into |
I think the underlying issue here is that dependency categorization in For a node application running on a server, splitting between For a client side application it is a bit more complex. It seems the status quo has more or less been:
and as of openedx/open-edx-proposals#615, the plan is to move to
I really wish
@bradenmacdonald re:
Considering If someone makes a breaking PR to I do think we should bring some more people into this discussion. openedx/open-edx-proposals#615 landed without any input from @arbrandes or @adamstankiewicz, and I didn't think through the implications of it as fully as I should have before approving it. Splitting up dependencies as
still feels semantically correct to me, but I understand the downsides of that strategy when taking build times into account. I think for implementing the strategy outlined in openedx/open-edx-proposals#615 to happen smoothly we'll need to clearly communicate the implications out to the greater Open edX frontend development community. |
I agree that continuing to include build-related dependencies in
|
FWIW, I did a bit of discovery around my earlier suggestion to treat {
"peerDependenciesMeta": {
"@openedx/frontend-build": {
"optional": true
}
}
} More details in the draft PR , but when installed into While |
Maybe I've got the wrong idea since the beginning: I'm now thinking that webpack, frontend-build et al. could remain in devDependencies, and then Tutor would provide its own build system, shared across MFEs, in a separate node_modules/ (spoiler: Tutor would then probably not use webpack at all). |
@regisb That sounds like a lot of work, and I don't think it's good to have two different build systems around for the same MFEs. Much as I'm a fan of ditching webpack and other goals like that. @brian-smith-tcril @adamstankiewicz
I agree that this feels correct to me too. However, because of how we build our MFEs, we don't "need" anything in the bundle when serving to users. The result of our build process is a bunch of static files in
But even though this makes sense and is fairly standard and familiar, it's not particularly useful. In this scenario, we never need just the As I understand, there are two potential problems affecting MFE builds: image size and slow dependency installation. The "normal" way to build a containerized version of an app like an MFE is: install all the dependencies, build the MFE, delete If |
According to my experiments, there is a third one, which is: slow build. And that's actually the most important one. Even after we trim dependencies, building the learning or the authoring apps still take more than one or two minutes, which seems totally bonkers to me. Here are the low-hanging fruit I've found:
Even after these build optimizations, I'm still getting terrible results:
This SUCKS. So I'm now looking at replacing webpack by rspack. Preliminary results are very promising, but I'm still facing issues with the build result. |
@regisb I think the main problem is that fast build time simply isn't something most React SPA developers optimize for at all. For example, I did a quick web search for "optimize react single page app for fast build" and found zero results related to build time - every result was about optimizing performance for users in the browser. Most React SPAs are designed to deploy to one production instance, meaning they only ever need to be built a couple times per change that lands on the branch that deploys to production. Once with any staging specific build time configuration, and once with production specific build time configuration. I'd argue the issue isn't one of build times, but one of how many times the MFEs need to be built. The design tokens project will at least remove the need to rebuild for theme changes. I'm completely in favor of finding ways to decrease build times so the status quo of needing to rebuild often is less painful, but I don't think we'll be able to get build times to a point where they can be considered good in a world where rebuilding MFEs is a common occurance. |
Here are a few resources related to improving build time: https://stackoverflow.com/questions/67615038/how-to-reduce-react-app-build-time-and-understanding-behaviour-of-webpack-when-b In addition, the cambrian explosion of build tools (webpack, vite, rsbuild, turobopack, farm, ...) tells us that there are many people out there who face the same problem as us. Build time does not affect only production builds, but development builds as well. An initial run of
MFEs need to be built locally, in CI, on production servers. This happens for every change in MFEs, and every change in an MFE plugin, for every user out there. We are building often because we are a community of many users with many different use cases. There is simply no way around it -- and design tokens are not going to resolve that at all. So we build often. And we can't put a 20 minute step with 400% CPU on the path of every build. This is a situation that has affected everyone in the community for years. We are now learning that the OEP-65 improvements are postponed from Teak to Ulmo. In the meantime, more MFEs are added to the community distribution. So I'm now doing the same thing that I did when I created Tutor, which is to rewrite the build configuration from scratch. I'm only ~50% confident this is going to be successful, but I'm pretty sure that in the process I'm going to learn some thing that can have at least a medium impact on build time. |
This one describes an issue with build time increasing significantly after adding
This one is mostly about performance for the user in the browser, with a small section about hot module replacement.
This one is about build times. It also links to https://webpack.js.org/guides/build-performance/, which has a note in the production section Warning Don't sacrifice the quality of your application for small performance gains! Keep in mind that optimization quality is, in most cases, more important than build performance. This one is interesting. Definitely focused on better build times, and suggests a few ways to dig into what is slowing down builds.
This one is about optimizing for smaller bundles so users don't need to download as much. Definitely a good thing to do, but it's also possible that the configuration that leads to the smallest bundle isn't the configuration that leads to the fastest build. This one is interesting. The section about build times links to a repo with a couple good examples of how to use some build time profiling tools, which is great!
This one has some reasonable suggestions. One thing that stood out to me was
but when looking into autodll-webpack-plugin I found the README began with
The linked part (Part 1) is basically just saying "exclude Part 2 is about moving away from That seems odd to me, and is probably worth investigating again.
Looking at the comparisons on https://rspack.dev/ shows decently large improvements for initial build times, but the hot module replacement times (how long it takes for a hot reload in the browser after making a change when running a dev server) are already quite low for webpack.
I understand that the current state of build times is painful, and I agree it should be improved. I am curious as to what a good build time would be in your mind. I don't doubt we can make improvements in this space, but I do have doubts that those improvements will be monumental. I am also concerned about focusing too heavily on build time. I don't want build time improvements to come at the cost of in-browser performance for users of the platform (learners, course authors, course staff). I also don't think we should rule out trying to reduce the number of situations that require site operators to rebuild MFEs. I in no way was trying to suggest that design tokens is a silver bullet, I just see it as an example of providing site operators with a way to customize the frontend without rebuilding. If we can do a similar thing for plugins (which I believe is one of the goals of
I'm excited to see what you find! As I mentioned earlier in this post, I have no doubt there are improvements we can make in this space. Thank you for digging in and looking for ways to make build times better! |
I just want to point out that I was only measuring the effects of removing babel on dependency size/count, not build speed. It seems I didn't write down the actual build times. |
Ah. That makes a lot more sense. Definitely worth revisiting to see the impact on build speed. |
In my experiments, replacing babel by esbuild has a considerable impact (roughly 50%), but it's not sufficient by itself (IMHO). |
This is to allow
npm install --omit=dev
across all MFE apps.