Cc/smaller bundle #3725

ChristopherChudzicki · 2022-09-06T20:08:41Z

Pre-Flight checklist

What are the relevant tickets?

What's this PR do?

This PR reduces the bundle size of open discussions and infinite corridor:

                        master      this branch
------------------------------------------------                        
Open Discussions:       7.5M        4.2M
Infinite Corridor:      5.8M        2.5M

Additionally, added webpack-bundle-analyzer to both webpack configs so that we can more easily see what is contributing to our bundle size.

How does this reduce bundle size?

Primarily, this reduces the bundle size by removing the factory functions from our local modules's top-level exports. So:

// Before:
import { makeLearningResource} from "ol-search-ui"
// Now:
import { makeLearningResource} from "ol-search-ui/build/factories"

See #3714 for more.

How should this be manually tested?

In the watch container, run NODE_ENV=production yarn run build.
Run ls -lh frontends/infinite-corridor/build and ls -lh frontends/open-discussions/build.
- Check that the JS bundles are smaller like I said. Check that there is
- Check that there is no report.html file
In the container again, run WEBPACK_ANALYZE=True NODE_ENV=production yarn run build.
Open frontends/infinite-corridor/build/report.html in your browser to see the bundle analysis.

Screenshots

This is the infinite corridor bundle report... There's still some work we could do, e.g., CKEditor is only used by moderators, I think it could be loaded dynamically. But that can be looked into separately.

codecov-commenter · 2022-09-06T21:30:33Z

Codecov Report

Merging #3725 (0ea64d0) into master (735d2dd) will decrease coverage by 0.00%.
The diff coverage is 82.35%.

@@            Coverage Diff             @@
##           master    #3725      +/-   ##
==========================================
- Coverage   86.89%   86.88%   -0.01%     
==========================================
  Files         329      329              
  Lines       13991    13988       -3     
  Branches     2301     2300       -1     
==========================================
- Hits        12157    12153       -4     
  Misses       1561     1561              
- Partials      273      274       +1

Impacted Files	Coverage Δ
frontends/ol-search-ui/src/components/Facet.tsx	`50.00% <ø> (ø)`
frontends/ol-search-ui/src/factories.ts	`50.98% <40.00%> (-0.95%)`	⬇️
...ends/infinite-corridor/src/api/fields/factories.ts	`100.00% <100.00%> (ø)`
...ontends/infinite-corridor/src/pages/SearchPage.tsx	`81.66% <100.00%> (ø)`
frontends/ol-forms/src/components/SelectField.tsx	`75.00% <100.00%> (ø)`
...src/components/ExpandedLearningResourceDisplay.tsx	`70.00% <100.00%> (ø)`
frontends/ol-search-ui/src/util.tsx	`64.94% <100.00%> (-0.71%)`	⬇️
frontends/ol-widgets/src/factories.ts	`76.31% <100.00%> (+0.64%)`	⬆️
search/api.py	`95.93% <0.00%> (-1.17%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ChristopherChudzicki · 2022-09-07T14:31:50Z

frontends/infinite-corridor/package.json

@@ -31,7 +31,6 @@
    "ol-util": "workspace:*",
    "ol-widgets": "workspace:*",
    "postcss-loader": "^7.0.1",
-    "ramda": "^0.28.0",


I also removed all usage of ramda in this PR because:

it was easy to remove. All of our usages have simple native JS alternatives or analogous lodash functions

we were including it twice in our bundle accidentally.

If we need it for something, we can definitely add it back.

ChristopherChudzicki · 2022-09-07T14:39:30Z

frontends/infinite-corridor/src/api/fields/factories.ts

-import * as R from "ramda"
-import { makePaginatedFactory, Factory } from "ol-util"
-import { LearningResourceType, factories } from "ol-search-ui"
+import { makePaginatedFactory, Factory } from "ol-util/build/factories"


I would very much have preferred an import from ol-util/factories instead of ol-util/build/factories. It's fairly easy to configure that with Node subpath exports.

But as far as I can tell, making that work with Typescript means moving to ESM-style module resolution, which is a fairly big change. So I went for the uglier ol-util/build/factories imports. From microsoft/TypeScript#33079 (comment) (emphasis mine):

TypeScript doesn’t yet have a resolution mode suitable for most bundlers, and package.json exports are not yet configurable independently of the module resolution mode on the whole. This is discussed at length in microsoft/TypeScript#50152.

Related: https://www.typescriptlang.org/docs/handbook/esm-node.html

ChristopherChudzicki · 2022-09-07T14:43:17Z

frontends/infinite-corridor/src/scss/fieldpage.scss

+  margin-left: $carouselSpacing*0.5;
+  margin-right: $carouselSpacing*0.5;


This is from running npx sass-migrator division **/*.scss, which fixed some deprecated Sass syntax.

(At one point on this branch I had a failing build that I thought might be related to a sass issue, so I was trying to clean up some warnings.)

ChristopherChudzicki · 2022-09-07T15:09:43Z

frontends/ol-util/src/lib/utils.test.ts

+  })
+
+  it("Returns true for numbers", () => {
+    // _.isEmpty(5) returns true; this is different from ramda.


If the different ever matters to us, I would be surprised.

Shinigami92 · 2022-09-08T07:42:45Z

frontends/ol-widgets/src/factories.ts

@@ -1,5 +1,6 @@
 import { faker } from "@faker-js/faker"


Suggested change

import { faker } from "@faker-js/faker"

import { faker } from "@faker-js/faker/locale/en"

As you are not changing the locale at runtime, just use the en locale

Shinigami92 · 2022-09-08T07:43:30Z

frontends/infinite-corridor/src/api/fields/factories.ts

@@ -1,7 +1,8 @@
 import { faker } from "@faker-js/faker"


Suggested change

import { faker } from "@faker-js/faker"

import { faker } from "@faker-js/faker/locale/en"

Aside for other mitodl folks: Shinigami92 is one of the faker-js maintainers and previously noted that using specific imports like this would reduce faker's footprint.

Personally, I'm somewhat inclined to keep the faker imports as is because:

faker should not influence the overall bundle size at all. The fact that faker was influencing our bundle size prior to this PR was a mistake.

the subpath import is a little more verbose. I'm not really confident that we'd use it consistently, and per ☝️ I do not understand the benefit.

Shinigami92: If I've misunderstood and there is some benefit I'm not seeing, feel free to let me know.

No you are right 👍
I would suggest to upgrade to v8 as soon as it's available

This would only be a benefit until we have managed better locale handling on our side.

Shinigami92 · 2022-09-08T07:44:24Z

frontends/ol-search-ui/src/factories.ts

@@ -1,9 +1,8 @@
 //@ts-expect-error casual-browserify does not have typescript types
 import casual from "casual-browserify"
 import { faker } from "@faker-js/faker"


Suggested change

import { faker } from "@faker-js/faker"

import { faker } from "@faker-js/faker/locale/en"

abeglova

LGTM

ChristopherChudzicki added 7 commits September 6, 2022 16:08

remove factory exports from root entry

909d589

autofix some sass deprecations

9b60b67

add webpack-bundle-analyzer

084f375

remove ramda

131c1e2

use lodash.times not ramda.times

af10fed

fix old factory import

e54e2cd

fix test

9cd41ed

add bundle tracker

0ea64d0

ChristopherChudzicki force-pushed the cc/smaller-bundle branch from a274e9e to 0ea64d0 Compare September 7, 2022 14:28

ChristopherChudzicki commented Sep 7, 2022

View reviewed changes

add a few simple tests

d417f2a

ChristopherChudzicki commented Sep 7, 2022

View reviewed changes

ChristopherChudzicki marked this pull request as ready for review September 7, 2022 15:23

ChristopherChudzicki added the Needs Review label Sep 7, 2022

Shinigami92 reviewed Sep 8, 2022

View reviewed changes

abeglova self-assigned this Sep 8, 2022

abeglova approved these changes Sep 8, 2022

View reviewed changes

ChristopherChudzicki merged commit 1ba85b0 into master Sep 8, 2022

odlbot mentioned this pull request Sep 8, 2022

Release 0.198.1 #3730

Merged

1 task

ChristopherChudzicki mentioned this pull request Sep 16, 2022

Bundle size is unnecessarily big (open discussions, infinite corridor) #3714

Closed

rhysyngsun deleted the cc/smaller-bundle branch March 10, 2023 18:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cc/smaller bundle #3725

Cc/smaller bundle #3725

ChristopherChudzicki commented Sep 6, 2022 •

edited

Loading

codecov-commenter commented Sep 6, 2022 •

edited

Loading

ChristopherChudzicki Sep 7, 2022

ChristopherChudzicki Sep 7, 2022

ChristopherChudzicki Sep 7, 2022

ChristopherChudzicki Sep 7, 2022

Shinigami92 Sep 8, 2022

Shinigami92 Sep 8, 2022

ChristopherChudzicki Sep 8, 2022 •

edited

Loading

Shinigami92 Sep 8, 2022 •

edited

Loading

Shinigami92 Sep 8, 2022

abeglova left a comment

		margin-left: $carouselSpacing*0.5;
		margin-right: $carouselSpacing*0.5;

	import { faker } from "@faker-js/faker"
	import { faker } from "@faker-js/faker/locale/en"

Cc/smaller bundle #3725

Cc/smaller bundle #3725

Conversation

ChristopherChudzicki commented Sep 6, 2022 • edited Loading

Pre-Flight checklist

What are the relevant tickets?

What's this PR do?

How should this be manually tested?

Screenshots

codecov-commenter commented Sep 6, 2022 • edited Loading

Codecov Report

ChristopherChudzicki Sep 7, 2022

Choose a reason for hiding this comment

ChristopherChudzicki Sep 7, 2022

Choose a reason for hiding this comment

ChristopherChudzicki Sep 7, 2022

Choose a reason for hiding this comment

ChristopherChudzicki Sep 7, 2022

Choose a reason for hiding this comment

Shinigami92 Sep 8, 2022

Choose a reason for hiding this comment

Shinigami92 Sep 8, 2022

Choose a reason for hiding this comment

ChristopherChudzicki Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

Shinigami92 Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

Shinigami92 Sep 8, 2022

Choose a reason for hiding this comment

abeglova left a comment

Choose a reason for hiding this comment

ChristopherChudzicki commented Sep 6, 2022 •

edited

Loading

codecov-commenter commented Sep 6, 2022 •

edited

Loading

ChristopherChudzicki Sep 8, 2022 •

edited

Loading

Shinigami92 Sep 8, 2022 •

edited

Loading