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

better tree-shaking... compile ts to ESM (before bundling) #1044

Merged
merged 1 commit into from
Jan 23, 2023

Conversation

ChristopherChudzicki
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki commented Jan 20, 2023

Pre-Flight checklist

  • Testing
    • Changes have been manually tested I have not tested that sentry actually reads these source maps; I think we can wait till for QA servers for that.

What are the relevant tickets?

Related to #1033, #1034

What's this PR do?

This PR changes the way we compile TS before being bundled by webpack: In our tsconfig, we had module set to CommonJS, whereas it should be set to ES6 As the Webpack Typescript docs say,

Warning
ts-loader uses tsc, the TypeScript compiler, and relies on your tsconfig.json configuration. Make sure to avoid setting module to "CommonJS", or webpack won't be able to tree-shake your code.

How should this be tested

There should be no noticeable changes, other than that the output of yarn build:webpack should be a bit smaller.

You could...

  1. Check the site still looks ok at on the netlify builds
  2. If you want:
    • Run yarn build:wepback. It should succeed and the js file sizes (ls -lh base-theme/dist/js) should be a bit smaller than they are on main branch.

    • Run a site in production mode on this branch, e.g., by

      1. Build a site for production. Assuming your ocw-content-rc and ocw-hugo-projects directories adjacent to ocw-hugo-themes, yarn build abs/path/to/9.40-spring-2018 abs/path/to/ocw-hugo-projects/ocw-course-v2/config.yaml
      2. Serve the built course: npx serve path/to/9.40-spring-2018/dist.
      3. Open the url that serve displayed in your terminal and check that the soruce code is viewable:

      Again, there should be no noticeable changes.

Background Info

The "module" config setting tells Typescript what module syntax to use when compiling TS files into JS files. Specifying ES6 means use ES6 import/export. Note: This ES6 import syntax does not end up in the final bundles. Again, from Webpack docs](https://webpack.js.org/concepts/manifest/):

No matter which module syntax you have chosen, those import or require statements have now become webpack_require methods that point to module identifiers.

The advantage of compiling TS to JS is better tree-shaking (removal of unused code). It's not a huge difference at the moment:

                commonjs    es6
main.js	        323 kb	    313 kb																							
www.js	        580 kb	    516 kb      <-- biggest difference																				
course_v2.js	866 kb	    864 kb																							

but it's free, and I expect in the future it could matter more. (For example: I came across this issue while working on #1037, which I have decided to set aside. While working on #1037 , I found that because of changes in how Sentry organizes their code in v7, we Sentry v7's footprint on our bundle would be 54kb instead of 20kb without this config change.)

@ChristopherChudzicki ChristopherChudzicki changed the title better tree-shaking... compile ts to ESM better tree-shaking... compile ts to ESM (before bundling) Jan 20, 2023
@github-actions
Copy link

github-actions bot commented Jan 20, 2023

Lighthouse results:

results for https://ocw-hugo-themes-www-pr-1044--ocw-next.netlify.app/:

Accessibility Best Practices Performance Progressive Web App SEO
100 💯 83 😄 59 😟 20 😱 79 🙂

results for https://ocw-hugo-themes-www-pr-1044--ocw-next.netlify.app/search/:

Accessibility Best Practices Performance Progressive Web App SEO
93 🎉 92 🎉 75 🙂 30 😰 93 🎉

results for https://ocw-hugo-themes-course-v2-pr-1044--ocw-next.netlify.app/:

Accessibility Best Practices Performance Progressive Web App SEO
80 😄 92 🎉 92 🎉 30 😰 76 🙂

@ChristopherChudzicki ChristopherChudzicki marked this pull request as ready for review January 20, 2023 18:17
@ChristopherChudzicki
Copy link
Contributor Author

ChristopherChudzicki commented Jan 20, 2023

If you're interested and want to see this in action with our config (but with something more readable than our actual bundles), you can check out https://github.com/mitodl/ocw-hugo-themes/tree/cc/ts-esm-experiment. That branch defines three new files (just for testing):

// example_dog.js
export const dog = 'dog'
export const woof = (n: number) => 'woof'.repeat(n)

// example_cat.js
export const cat = 'cat'
export const meow = (n: number) => 'meow'.repeat(n)

// example_entry.js imports from cat and dog
// but does not use all of cat
import { cat } from "./example_cat"
import { dog, woof } from "./example_dog"

console.log(cat, "...?")

console.log(dog, woof(3))

Note that the meow function is unused. If you run the webpack build:

  • if tsconfig module: commonjs, meow is included in the build output
  • if tsconfig module: ES6, meow will be omitted

@pt2302 pt2302 self-requested a review January 20, 2023 19:46
@pt2302 pt2302 self-assigned this Jan 20, 2023
tldr: Changing `module` config changes the syntax used to import/export in the compiled code. The new value, "ES6", means that the code compiled by TS will include import/export statements. That code is then further compiled by Webpack, which removes the imports/exports during concatenation.

Using ES6 import/exports in the typescript output improves Webpack's tree-shaking. (Or really...enables it. I believe webpack does not treeshake unless the input uses ES6 import/export syntax.)

Related:

TS has several sort of settings related to the syntax used in output:

- `lib`: determines the default/builtin type definitions that are included when typechecking your code.
- `target`: Spcecies environment in which your code is going to run, and controls how Typescript downlevels your code.

    > "The `target` setting changes which JS features are downleveled and which are left intact. For example, an arrow function `() => this` will be turned into an equivalent function expression if target is ES5 or lower."

    `target` determines the default value of `lib`. If `target` is ES5, you probably want `lib` to be ES5, too, unless you're doing further transpilation with another tool (like babel).
- `module`: Determines the syntax used for exports.
- `moduleResolution` determines how module identifiers ("some/thing" in  like `import x from "some/thing"`) are interpretted.
Copy link
Contributor

@pt2302 pt2302 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants