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

build: Switch from CJS to ESM #1160

Merged
merged 10 commits into from Dec 8, 2022
Merged

build: Switch from CJS to ESM #1160

merged 10 commits into from Dec 8, 2022

Conversation

samestep
Copy link
Collaborator

@samestep samestep commented Dec 8, 2022

Description

While working on #1092, I realized that it would be very nice to have access to top-level await. However, that is only supported by ESM and not CJS, so I decided to first open this PR to migrate us to ESM.

Implementation strategy and design decisions

This was mostly just a lot of trial-and-error, digging through documentation, and playing with config options until it worked. I got a lot of help from the following threads:

By volume, most of the code changes in this PR are just switching imports of CJS modules to only use the default import rather than trying to destructure it, because apparently the latter is not actually supported when importing a CJS module from an ESM module.

One other change: because we need the global-jsdom import to come first in automator and synthesizer, I upgraded us to TypeScript 4.9.4 and prettier-plugin-organize-imports 3.2.1, to give us grouped imports which were added in TypeScript 4.7. These upgrades caused Prettier to start trying to remove a bunch of React imports, so I added "organizeImportsSkipDestructiveCodeActions": true to our .prettierrc.

Examples with steps to reproduce them

I manually verified that this works by running yarn start with a top-level await in packages/core/src/index.ts, but removed that before submitting this PR because we don't actually use it yet. Note that to get that to actually work, you have to also set the build target to ESNext for all our Vite packages:

diff --git a/packages/components/vite.config.ts b/packages/components/vite.config.ts
index 82adb3959..994c5f3c2 100644
--- a/packages/components/vite.config.ts
+++ b/packages/components/vite.config.ts
@@ -6,6 +6,7 @@ import { defineConfig } from "vite";
 export default defineConfig({
   plugins: [react()],
   build: {
+    target: "esnext",
     sourcemap: true,
     lib: {
       name: "@penrose/components",
diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts
index db938c845..6341541b5 100644
--- a/packages/core/src/index.ts
+++ b/packages/core/src/index.ts
@@ -35,6 +35,8 @@ import {
   zip2,
 } from "./utils/Util";
 
+await new Promise((r) => setTimeout(r, 1));
+
 /**
  * Use the current resample seed to sample all shapes in the State.
  * @param state current state
diff --git a/packages/editor/vite.config.ts b/packages/editor/vite.config.ts
index 05d39677e..a57d79c24 100644
--- a/packages/editor/vite.config.ts
+++ b/packages/editor/vite.config.ts
@@ -5,4 +5,5 @@ import { defineConfig } from "vite";
 export default defineConfig({
   base: "/try/",
   plugins: [react()],
+  build: { target: "esnext" },
 });
diff --git a/packages/synthesizer-ui/vite.config.ts b/packages/synthesizer-ui/vite.config.ts
index 415a94cd8..4e49cbac6 100644
--- a/packages/synthesizer-ui/vite.config.ts
+++ b/packages/synthesizer-ui/vite.config.ts
@@ -8,4 +8,5 @@ export default defineConfig({
   optimizeDeps: {
     exclude: ["@penrose/core", "@penrose/components"],
   },
+  build: { target: "esnext" },
 });

Checklist

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new ESLint warnings
  • I have reviewed any generated changes to the diagrams/ folder

Open questions

Although yarn start:docs-site works with this PR, it does not work after applying the top-level await diff given above. Rather, it crashes with this error message:

Compiled with problems:X

ERROR in ../components/dist/index.js

Module parse failed: The top-level-await experiment is not enabled (set experiments.topLevelAwait: true to enabled it)

I propose to merge this PR now and then figure out how to do top-level await with Docusaurus in a followup.

@codecov
Copy link

codecov bot commented Dec 8, 2022

Codecov Report

Merging #1160 (d78f750) into main (10c9736) will decrease coverage by 2.49%.
The diff coverage is 72.22%.

@@            Coverage Diff             @@
##             main    #1160      +/-   ##
==========================================
- Coverage   64.56%   62.07%   -2.50%     
==========================================
  Files          59       59              
  Lines        7677     7111     -566     
  Branches     1714     1708       -6     
==========================================
- Hits         4957     4414     -543     
+ Misses       2617     2614       -3     
+ Partials      103       83      -20     
Impacted Files Coverage Δ
packages/core/src/contrib/Functions.ts 27.74% <0.00%> (-1.21%) ⬇️
packages/core/src/contrib/Utils.ts 41.81% <ø> (-3.95%) ⬇️
packages/core/src/engine/Autodiff.ts 84.66% <ø> (-0.42%) ⬇️
packages/core/src/engine/EngineUtils.ts 52.20% <ø> (-2.73%) ⬇️
packages/core/src/renderer/Path.ts 3.84% <0.00%> (-5.68%) ⬇️
packages/core/src/utils/CollectLabels.ts 77.90% <ø> (-1.29%) ⬇️
packages/core/src/utils/Graph.ts 39.65% <ø> (-27.56%) ⬇️
packages/core/src/utils/Util.ts 60.05% <0.00%> (-0.97%) ⬇️
packages/core/src/synthesis/Synthesizer.ts 26.75% <23.52%> (-1.66%) ⬇️
packages/core/src/parser/ParserUtil.ts 98.78% <66.66%> (+1.19%) ⬆️
... and 64 more

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

@cloudflare-pages
Copy link

cloudflare-pages bot commented Dec 8, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: a69a768
Status: ✅  Deploy successful!
Preview URL: https://5c18d2e6.penrose-72l.pages.dev
Branch Preview URL: https://esm.penrose-72l.pages.dev

View logs

@samestep samestep requested a review from wodeni December 8, 2022 05:55
@samestep samestep marked this pull request as ready for review December 8, 2022 06:02
@github-actions
Copy link

github-actions bot commented Dec 8, 2022

📊 Performance

Key

Note that each bar component rounds up to the nearest 100ms, so each full bar is an overestimate by up to 400ms.

     0s   1s   2s   3s   4s   5s   6s   7s   8s   9s
     |    |    |    |    |    |    |    |    |    |
name ▝▀▀▀▀▀▀▀▀▀▀▀▚▄▄▄▄▄▄▄▄▄▞▀▀▀▀▀▀▀▀▀▀▀▀▚▄▄▄▄▄▄▄▄▄▖
      compilation labelling optimization rendering

Data

                                        0s   1s   2s   3s   4s   5s   6s   7s   8s   9s  10s  11s  12s  13s  14s  15s  16s  17s  18s  19s  20s  21s  22s  23s  24s  25s  26s  27s  28s  29s  30s  31s  32s  33s  34s  35s  36s  37s  38s  39s  40s  41s  42s  43s  44s  45s  46s  47s  48s  49s  50s  51s  52s  53s  54s  55s  56s  57s  58s  59s  60s  61s  62s  63s  64s  65s  66s  67s  68s  69s  70s  71s  72s  73s  74s  75s  76s  77s  78s  79s  80s  81s  82s  83s  84s  85s  86s  87s  88s  89s
                                        |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |    |
3d-projection-fake-3d-linear-algebra    ▝▚▚
allShapes-dashedShapes                  ▝▞▖
arrowheads-arrowheads                   ▝▀▞▖
circle-example-euclidean                ▝▀▀▀▀▀▞▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▖
collinear-euclidean                     ▝▀▀▞▀▀▚
congruent-triangles-euclidean           ▝▀▀▀▀▀▀▀▞▚
continuousmap-continuousmap             ▝▀▞▖
hypergraph-hypergraph                   ▝▀▀▀▀▀▀▀▀▀▀▀▚▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▚
incenter-triangle-euclidean             ▝▀▀▀▞▀▀▀▀▀▀▀▀▚
lagrange-bases-lagrange-bases           ▝▀▞▖
midsegment-triangles-euclidean          ▝▀▀▚▚
non-convex-non-convex                   ▝▀▀▀▞▀▀▀▀▚
one-water-molecule-atoms-and-bonds      ▝▚▚
parallel-lines-euclidean                ▝▀▀▞▀▀▀▀▀▀▀▀▚
persistent-homology-persistent-homology ▝▀▀▀▀▀▀▞▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▄
points-around-line-shape-distance       ▝▀▀▀▀▀▞▖
points-around-polyline-shape-distance   ▝▀▀▀▀▀▀▀▀▀▀▀▞▀▖
points-around-star-shape-distance       ▝▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▚▀▖
siggraph-teaser-euclidean-teaser        ▝▀▀▀▚▀▀▚
small-graph-disjoint-rect-line-horiz    ▝▀▀▀▀▀▀▀▀▚▀▖
small-graph-disjoint-rects              ▝▀▞▖
small-graph-disjoint-rects-large-canvas ▝▀▞▖
small-graph-disjoint-rects-small-canvas ▝▀▞▖
tree-tree                               ▝▀▚▚
tree-venn                               ▝▀▀▚▀▚
tree-venn-3d                            ▝▀▀▀▞▀▀▄▖
two-vectors-perp-vectors-dashed         ▝▀▞▖
vector-wedge-exterior-algebra           ▝▀▚▚
wet-floor-atoms-and-bonds               ▝▀▀▀▚▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▚
wos-laplace-estimator-walk-on-spheres   ▝▀▀▀▞▀▀▚
wos-nested-estimator-walk-on-spheres    ▝▀▀▀▀▚▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▚
wos-offcenter-estimator-walk-on-spheres ▝▀▀▀▞▀▀▀▚
wos-poisson-estimator-walk-on-spheres   ▝▀▀▀▞▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▖

Copy link
Member

@wodeni wodeni left a comment

Choose a reason for hiding this comment

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

LGTM! lodash should really just be esm 🤡

@samestep samestep merged commit 3ae696e into main Dec 8, 2022
@samestep samestep deleted the esm branch December 8, 2022 17:02
@wodeni wodeni mentioned this pull request Dec 8, 2022
3 tasks
This was referenced Dec 9, 2022
@samestep samestep mentioned this pull request Jan 1, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants