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: monorepo-aware reloading in editor #1286

Merged
merged 4 commits into from Feb 6, 2023
Merged

build: monorepo-aware reloading in editor #1286

merged 4 commits into from Feb 6, 2023

Conversation

wodeni
Copy link
Member

@wodeni wodeni commented Feb 2, 2023

Description

Previously, we had trouble watching changes from packages in our monorepo. For instance, a change in @penrose/core will not trigger a reload when running yarn start because it only watches source file changes within @penrose/editor.

After #1160, all of our packages are ES modules and vite is capable of treating ESM dependencies as source. This PR configures @penrose/editor such that it watches changes from both core and components.

Implementation strategy and design decisions

  • Top-level await: since we are treating core as source, editor's vite config also needs to know about how to deal with top level await. Added the suggested plugin.
  • Absolute path resolution: vite is not going to run custom plugins such as ttsc in core. Therefore, we need to make sure core has minimal configuration. This PR removes ttsc from core and changed all absolute path imports to relative.
    • it doesn't look that bad. Most imports are just ../<module>/file, which is pretty logical.

Examples with steps to reproduce them

Run yarn start. Touch any files in core or components to trigger HMR in vite.

Checklist

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

Open questions

This issue was very helpful for this PR, but I couldn't reproduce the regex-based approach because examples and optimizer have different folder structures. I'm inclined to keep the aliases explicit because it's much more transparent.

@wodeni wodeni requested a review from samestep February 2, 2023 05:38
@github-actions
Copy link

github-actions bot commented Feb 2, 2023

± Registry diff

📊 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
                                                   |    |    |    |    |    |    |    |
3d-projection-fake-3d-linear-algebra               ▝▚▚
allShapes-allShapes                                ▝▞▄▄▄
arrowheads-arrowheads                              ▝▞▖
caffeine-structural-formula                        ▝▀▀▞▚
center-shrink-circle-animation                     ▝▞▖
circle-example-euclidean                           ▝▀▞▀▀▚
collinear-euclidean                                ▝▀▞▖
congruent-triangles-euclidean                      ▝▀▚▚
continuousmap-continuousmap                        ▝▞▖
cubic-bezier-cubic-bezier                          ▝▚▀▀▖
glutamine-molecules-basic                          ▝▀▞▖
half-adder-distinctive-shape                       ▝▚▚
hypergraph-hypergraph                              ▝▀▀▚▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▚
incenter-triangle-euclidean                        ▝▀▞▖
lagrange-bases-lagrange-bases                      ▝▚▚
midsegment-triangles-euclidean                     ▝▀▞▖
mobius-mobius                                      ▝▞▖
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                  ▝▀▀▀▞▖
quaternion-group-group-theory-cayley-graph         ▝▀▞▖
quaternion-group-group-theory-multiplication-table ▝▀▀▚▞▖
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-triangles-triangle-mesh-3d                     ▝▚▚
two-vectors-perp-vectors-dashed                    ▝▚▚
vector-wedge-exterior-algebra                      ▝▞▖
wet-floor-atoms-and-bonds                          ▝▚▀▀▖
word-cloud-example-word-cloud                      ▝▀▞▖
wos-laplace-estimator-walk-on-spheres              ▝▀▞▖
wos-nested-estimator-walk-on-spheres               ▝▀▀▞▀▀▀▀▀▀▚
wos-offcenter-estimator-walk-on-spheres            ▝▀▞▀▀▀▖
wos-poisson-estimator-walk-on-spheres              ▝▀▞▀▖

@codecov
Copy link

codecov bot commented Feb 2, 2023

Codecov Report

Merging #1286 (c93e960) into main (115a609) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1286   +/-   ##
=======================================
  Coverage   63.43%   63.43%           
=======================================
  Files          61       61           
  Lines        7212     7212           
  Branches     1680     1680           
=======================================
  Hits         4575     4575           
  Misses       2553     2553           
  Partials       84       84           
Impacted Files Coverage Δ
packages/core/src/analysis/SubstanceAnalysis.ts 74.85% <ø> (ø)
packages/core/src/compiler/Domain.ts 84.02% <ø> (ø)
packages/core/src/compiler/Style.ts 68.51% <ø> (ø)
packages/core/src/compiler/Substance.ts 91.98% <ø> (ø)
packages/core/src/contrib/Constraints.ts 82.88% <ø> (ø)
packages/core/src/contrib/ConstraintsUtils.ts 65.38% <ø> (ø)
packages/core/src/contrib/CurveConstraints.ts 100.00% <ø> (ø)
packages/core/src/contrib/Functions.ts 27.09% <ø> (ø)
packages/core/src/contrib/ImplicitShapes.ts 100.00% <ø> (ø)
packages/core/src/contrib/Minkowski.ts 98.16% <ø> (ø)
... and 38 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 Feb 2, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: c93e960
Status: ✅  Deploy successful!
Preview URL: https://2573faa3.penrose-72l.pages.dev
Branch Preview URL: https://vite-link.penrose-72l.pages.dev

View logs

@samestep
Copy link
Collaborator

samestep commented Feb 6, 2023

  • Absolute path resolution: vite is not going to run custom plugins such as ttsc in core. Therefore, we need to make sure core has minimal configuration. This PR removes ttsc from core and changed all absolute path imports to relative.
    • it doesn't look that bad. Most imports are just ../<module>/file, which is pretty logical.

Related: #1189

Copy link
Collaborator

@samestep samestep left a comment

Choose a reason for hiding this comment

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

Amazing, thanks for doing this Nimo! One thing to think about: now the only source package this doesn't work for is @penrose/optimizer which has a more complicated build process that Vite doesn't understand, which is why we still need "dependsOn": ["^build"] for the three dev tasks packages/editor/package.json. But those tasks don't actually depend on core:build or components:build, just optimizer:build. Doesn't really matter though :)

@wodeni wodeni merged commit d5181cf into main Feb 6, 2023
@wodeni wodeni deleted the vite-link branch February 6, 2023 21:20
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