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: don't bundle core #1173

Merged
merged 19 commits into from Jan 17, 2023
Merged

build: don't bundle core #1173

merged 19 commits into from Jan 17, 2023

Conversation

samestep
Copy link
Collaborator

@samestep samestep commented Dec 14, 2022

Description

We don't need to, as far as I can tell; anything using @penrose/core that wants to bundle it should be able to just bundle it directly, so there's no reason to put all our dependencies into the bundle when they're already dependencies of the package itself. This is a followup to #1160, #1165, and #1170. Closes #423.

Implementation strategy and design decisions

To increase consistency with our other packages, I changed the core build directory from build/dist/ to just dist/. Previously packages/synthesizer-ui/README.md also erroneously said it used build/ instead of dist/, so I corrected that here too.

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

@cloudflare-pages
Copy link

cloudflare-pages bot commented Dec 14, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: e391970
Status:⚡️  Build in progress...

View logs

@codecov
Copy link

codecov bot commented Dec 14, 2022

Codecov Report

Merging #1173 (a49aaa9) into main (2339d93) will increase coverage by 0.00%.
The diff coverage is 50.00%.

❗ Current head a49aaa9 differs from pull request most recent head e391970. Consider uploading reports for the commit e391970 to get more accurate results

@@           Coverage Diff           @@
##             main    #1173   +/-   ##
=======================================
  Coverage   63.13%   63.13%           
=======================================
  Files          59       60    +1     
  Lines        7060     7061    +1     
  Branches     1646     1646           
=======================================
+ Hits         4457     4458    +1     
  Misses       2519     2519           
  Partials       84       84           
Impacted Files Coverage Δ
packages/core/src/renderer/Image.ts 3.84% <0.00%> (ø)
packages/core/src/renderer/not_found.ts 100.00% <100.00%> (ø)

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

@github-actions
Copy link

github-actions bot commented Dec 14, 2022

± 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             ▝▚▚
full-moon-full-moon                     ▝▀▀▀▚▀▀▚
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       ▝▀▀▀▞▖
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               ▝▚▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▚
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   ▝▀▀▞▀▖

@samestep
Copy link
Collaborator Author

Currently this gives the following error in editor:

Uncaught TypeError: Cannot read properties of undefined (reading 'prototype')
    at sax.js:221:46
    at ../../node_modules/xmldoc/node_modules/sax/lib/sax.js (sax.js:1574:1)
    at __require2 (chunk-5537U2Q2.js?v=ad4963f6:47:50)
    at xmldoc.js:7:9
    at ../../node_modules/xmldoc/lib/xmldoc.js (xmldoc.js:272:1)
    at __require2 (chunk-5537U2Q2.js?v=ad4963f6:47:50)
    at ../../node_modules/xmldoc/index.js (index.js:3:18)
    at __require2 (chunk-5537U2Q2.js?v=ad4963f6:47:50)
    at ../../node_modules/svg-flatten/src/parse.js (parse.js:1:14)
    at __require2 (chunk-5537U2Q2.js?v=ad4963f6:47:50)

@samestep samestep mentioned this pull request Jan 11, 2023
3 tasks
@samestep samestep marked this pull request as ready for review January 13, 2023 14:03
@samestep samestep changed the title build: Don't bundle core build: don't bundle core Jan 13, 2023
@samestep samestep requested a review from wodeni January 13, 2023 14:03
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.

Looks good! With this and #1207 merged both core and components would be bundleless!

re yarn start: it's still quite useful to run multiple builds in watch mode. I think we can avoid tsc-alias by using ttypescript. I can push a commit for that soon.

@samestep samestep merged commit de0f455 into main Jan 17, 2023
@samestep samestep deleted the no-core-bundle branch January 17, 2023 17: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.

Reduce bundle size of @penrose/core
2 participants