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

Separate grapher and the OWID site into separate standalone projects in the same repo #1157

Closed
8 tasks
marcelgerber opened this issue Feb 14, 2022 · 5 comments
Closed
8 tasks

Comments

@marcelgerber
Copy link
Member

marcelgerber commented Feb 14, 2022

Tasks

  • Update our codebase to using ES Modules (i.e. TS emits ESM, not CommonJS). Benefits:
  • Update to Webpack 5
  • To discuss: 'Merge' grapher + coreTable + clientUtils
    • Benefit: publishing and versioning will become easier since we (at first) will only have one published package
  • To discuss: What to do about the existing settings module?
    • Problem: grapher depends on it, but it also contains settings for the DB, admin server, server-side secrets etc.
  • Switch over to using Yarn Workspaces + TS Project References
    • Also, switch to using Webpack with ts-loader
    • Packages will reside in a packages/ folder
    • Ensure that important scripts (local development, deploy) still work
  • Factor out the grapher module into its own package
  • Stretch goal: Use a ts-node-based setup

Why

  • Separation of concerns (it's clear that visualisation fixes should go into Grapher, and Admin stuff should not go into grapher)
  • Reusability and easier development workflows (e.g. development without database setup), see Project: make grapher and site standalone projects within repo #1082
  • Each package has its own dependencies, so it's clearer what is being used where and the Grapher will only need a small subset of dependencies

Risks

  • Potentially, if every package declares its own dependencies, dependency upgrades become harder to do for shared dependencies (e.g. TypeScript, lodash)
@marcelgerber
Copy link
Member Author

some justification on why there's no visible progress at all:

What did I do?

I decided it would be a good first step to convert our codebase to fully using ES modules, and thought that this would be an easy and sensible first step. It was not.

What are ES modules?

There are a number of different formats in which JS modules can be published. For a long time, CommonJS (CJS) modules were the norm since they were the only format recognized by Node.
They also worked in browsers, but only through the use of a build tool like Webpack (since the browser doesn't know what require is). You can recognize CommonJS code by the use of require() and module.exports.

But along came ES Modules (ESM for short), a standard that works in both the browser and in Node. It is natively supported in Node 12.20+. It was meant to instantaneously fix all the weirdnesses the JS packaging has. ESM uses the import and export keywords.

In our case, since our code is written in TypeScript, we are already using the ESM syntax, but tsc will transpile it to CommonJS syntax. This doesn't let us use the advantages that ES modules have.

The Upside: Why ES modules are great

In principle, ES modules are great. They will, at some point (it's still JavaScript), be perfectly supported by all browsers and tools. Their syntax is more sensible (mostly). It allows for parallel download of sources at parse-time. Tools like Webpack can do way better tree-shaking with ESM sources.

Additionally, many packages are now published as ESM-only. One very relevant example is the latest version of d3. We can only upgrade to that version if we ourselves upgrade to ESM.

The Downside: Why ES modules are terrible

Well, it's not necessarily the ESM design that is to blame. It's the compatibility and tooling around it. You see, you can never import an ESM module from CommonJS code, but it's perfectly possible to import CommonJS from ESM code. And since the whole world didn't switch over to ESM in one day, and a lot of our node_modules will still be written using CommonJS, we desperately need this kind of compatibility. This means that all our tools - Node, Webpack, TS, ... - will have to agree on how best to handle modules. They don't.

In fact, many modules are published as dual modules: They contain both an ESM and a CJS entry point, and specify these in package.json. They do this either by using the legacy module key in package.json, or by using the newfangled exports field. Webpack supports both of them, Node only recognizes exports. This already means that you will use different assets in the built Webpack assets than you will if you run the same in Node. This still wouldn't be a dealbreaker if it wasn't for Node to load default exports differently depending on if it's CJS or ESM.

Consider a simple

import Select from "react-select"

const render = () => <Select />

This will work fine if react-select is imported as an ESM module, but it won't if react-select is CJS. In that case, you would have to write <Select.default /> instead.
This is incredibly dumb and frustrating. People wanted to change it, but Node didn't want to go along.

The only alternatives I could come up with are:

  • use import Select from "react-select/dist/react-select.esm.js" instead, forcibly importing the ESM version. TypeScript enters the room and wants to kill you (it can't find types).
  • change all affected packages to specify an exports field so that Node will always use the ESM version. duh.
  • we could probably add another transpiler to our toolbelt, like babel or https://github.com/mgenware/ts-transform-esm-import.

See for yourself

If you're brave, you can take a look at the shipwreck itself, at https://github.com/owid/owid-grapher/tree/webpack5-esm. There is an immense number of fixup commits in there.

Many things work, some are extremely hacky, some don't work at all. For example, the SVG tester doesn't work and is hard to fix since it uses the multiprocessing package which uses dynamic require calls.

@marcelgerber marcelgerber removed their assignment Mar 8, 2022
@marcelgerber
Copy link
Member Author

Progress report:

I rebased https://github.com/owid/owid-grapher/tree/webpack5-esm to the latest master.
Just to make the scope of this clear: Basically the only impactful change on that branch is the following tsconfig.json change:

diff --git a/devTools/tsconfigs/tsconfig.base.json b/devTools/tsconfigs/tsconfig.base.json
--- a/devTools/tsconfigs/tsconfig.base.json
+++ b/devTools/tsconfigs/tsconfig.base.json
@@ -1,7 +1,7 @@
 {
     "$schema": "https://json.schemastore.org/tsconfig",
     "compilerOptions": {
-        "module": "commonjs",
+        "module": "es2020",
         "strict": true,

What it does is to instruct TS to output ESM code (rather than CommonJS) in the itsJustJavascript folder. That ESM is then further consumed by both Node and Webpack, with several problems stemming from that :)
However, most of these problems should be resolved. Problems that still exist are:

  • There are four packages where I had to patch the respective package.jsons to make it work. 🎉 I could get rid of that hack and could replace it with a slightly better hack!
  • The svgTester is not working, but @danyx23 is gonna fix that soon.
  • Some other devTools might also be broken.
  • Some tests and test suites are currently skipped.
  • Some failures we can only detect at runtime.

@marcelgerber
Copy link
Member Author

In other good news, the deploy was successful and the branch is now live on tufte.

@danyx23
Copy link
Contributor

danyx23 commented Sep 13, 2022

@larsyencken
Copy link
Contributor

Closing this issue, since the previous approach hit a dead end with ESM. Superseded by #1667

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

No branches or pull requests

4 participants