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

Don't include Learning Assistant in the bundle #1482

Open
bradenmacdonald opened this issue Sep 12, 2024 · 6 comments
Open

Don't include Learning Assistant in the bundle #1482

bradenmacdonald opened this issue Sep 12, 2024 · 6 comments
Labels
business-specific Code that relates to a specific user and should be refactored and removed. code health Proactive technical investment via refactorings, removals, etc.

Comments

@bradenmacdonald
Copy link
Contributor

bradenmacdonald commented Sep 12, 2024

To reduce the bundle size for everyone, we need to make a few fixes to https://github.com/edx/frontend-lib-learning-assistant :

  1. It's peerDependencies are overly specified. For example, it shouldn't pin redux to the exact version 4.1.2 as that blocks us from bumping the version in frontend-app-learning. Specifying ^4 or ^4.1 is fine.
  2. It should not be part of frontend-app-learning's redux store. Instead it should have its own store and its own <Provider>. Or better yet, don't use redux at all.
  3. The <Xpert> component inside <Chat> should use React.lazy so that the learning-assistant code is only loaded on demand, and not always bundled into the main learning MFE app bundle.
  4. Optional: It should be a plugin using frontend-plugin-framework, and not even mentioned in the code at all.

Numbers 2 and 3 are the main thing I'd like to achieve here, as having both of those will result in a bundle size reduction.

@bradenmacdonald bradenmacdonald added code health Proactive technical investment via refactorings, removals, etc. business-specific Code that relates to a specific user and should be refactored and removed. labels Sep 12, 2024
@bradenmacdonald
Copy link
Contributor Author

FYI @alangsto @MichaelRoytman - not quite sure who to ping about this. Could you let me know if you have any existing plans along these lines?

@alangsto
Copy link
Contributor

@bradenmacdonald I will bring this up to our team on Monday and get back to you! Thanks for pinging us.

@alangsto
Copy link
Contributor

Hi @bradenmacdonald, thank you again for bringing this to our attention! Two clarifying questions:

  • Why does the bundle size matter? Wondering mostly to help us prioritize where this work should fall
  • Is there a timeline associated with reducing the bundle size?

Thanks!

@bradenmacdonald
Copy link
Contributor Author

@alangsto Bundle size is one of the main factors affecting performance, and especially how quickly the Learning MFE will load for users on their first visit, and on subsequent visits after a new version has been deployed. Basically at the moment, users who are visiting any site other than edx.org are still downloading the frontend code for Xpert / Learning Assistant even though that's an edx-specific feature. So by moving it to be "lazy-loaded", those users will see the MFE loading faster because they no longer need to wait to download that code before it's initialized.

In this case, I would not say it's very high priority as the code size is not that big. But we want to get in the habit of keeping the bundle size small and using code-splitting (lazy loading) since those are best practices. That's why I'm adding BundleWatch to the repo as recommended by OEP-67, and tools like BundlePhobia and webpack-bundle-analyzer exist (the latter is already built in to our build).

What is high priority for me though is not particularly related, but the fact that the peerDependencies of frontend-lib-learning-assistant are blocking some dependency version bumps, so I'd love to see those updated sooner. I can open a PR to do that though.

@alangsto
Copy link
Contributor

@bradenmacdonald thank you for the added context, this is very helpful! I'm going to create a discovery ticket for my team to look into the options you listed to determine the scope. Once we have completed the discovery I will share that doc with you.

@bradenmacdonald
Copy link
Contributor Author

@alangsto Great, thanks! I should also mention, it will improve performance for everyone on edx.org as well, if they're not in a course/context that's using learning assistant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
business-specific Code that relates to a specific user and should be refactored and removed. code health Proactive technical investment via refactorings, removals, etc.
Projects
None yet
Development

No branches or pull requests

2 participants