source-academy / modules

Modules that can be imported by programs in Source Academy, an online experiential environment for computational thinking
Apache License 2.0
8 stars 28 forks source link

Fix Broken Patch for CSG After ESBuild Bump #285

Closed joeng03 closed 7 months ago

joeng03 commented 7 months ago

Description

This fixes 2 Issues:

  1. The @jscad/stl-serializer package depends on @jscad/modeling. It will resolve the version of @jscad/modeling to 2.9.11. To reduce bundle size and standardize the version of @jscad/modeling at 2.9.6 for the patch, I downgraded the version of @jscad/stl-serializer.

  2. The esbuild version bump seemed to have failed the function call to toPolygons(geometry1, colorizePolygons = true). Passing in colorizePolygons = true is incorrect JavaScript syntax, initially written with named parameters in mind (but named parameters are not supported in JavaScript). It runs and produces the expected output without throwing a ReferenceError before the esbuild version bump. I have rectified the syntax and created a new patch.

Fixes #265

Type of change

Please delete options that are not relevant.

Checklist:

joeng03 commented 7 months ago

Did you manually edit the lockfile? Can we just use a dependency resolution in package.json instead to ensure it doesn't accidentally get updated automatically?

Which lockfile did you mean, the .patch file or yarn.lock? I didn't manually edit either one

RichDom2185 commented 7 months ago

Did you manually edit the lockfile? Can we just use a dependency resolution in package.json instead to ensure it doesn't accidentally get updated automatically?

Which lockfile did you mean, the .patch file or yarn.lock? I didn't manually edit either one

I mean yarn.lock.

It will resolve the version of @jscad/modeling to 2.9.11. To reduce bundle size and standardize the version of @jscad/modeling at 2.9.6 for the patch

Are we keeping it downgraded for the sole purpose of resolution then? Why does moving beyond 2.9.6 break things?

joeng03 commented 7 months ago

yarn.lock was updated by running the yarn install command, not manually.

Since the patch file applies the changes on fixed line numbers, it is hard to guarantee that future versions will not cause the patched package to break due to wrongly replacing newly added code.

RichDom2185 commented 7 months ago

Since the patch file applies the changes on fixed line numbers, it is hard to guarantee that future versions will not cause the patched package to break due to wrongly replacing newly added code.

So we only need to freeze @jscad/modeling, right? Is there a reason to freeze @jscad/stl-serializer?

joeng03 commented 7 months ago

The latest version of@jscad/stl-serializer depends on @jscad/modeling but of a different version (2.9.11), so it may be better to standardize the version to 2.9.6