Move version-embed fn and embed in each package #2259
Conversation
|
Size Change: +4.58 kB (+0.53%) Total Size: 871 kB
ℹ️ View Unchanged
|
jeremywiebe
force-pushed
the
jer/version-embed
branch
from
258ba64 to
2a63ed8
Compare
… version in itself (slightly larger bundle size)
jeremywiebe
force-pushed
the
jer/version-embed
branch
from
2a63ed8 to
58d0a7f
Compare
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (29a8812) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR2259If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR2259 |
jeremywiebe
requested review from
benchristel,
handeyeco,
Myranae and
a team
| @@ -0,0 +1 @@ | |||
| ../../../utils/shared | |||
There was a problem hiding this comment.
What do all these files and lines do?
There was a problem hiding this comment.
I think they're symlinks to the utils/shared directory.
My understanding is that he wants to share the logic across packages, but doesn't want the logic to be in a package (to avoid circular dependencies). So each package has a "directory" with the logic, but really the directory is just pointing to a different place.
There was a problem hiding this comment.
Exactly that. That's how git represents symlinks. By using a directory that's symlinked in... we can share the code, but it's just copied into each bundle instead of being referenced as a package dependency.
| // ESLint's import/no-relative-packages doesn't understand symlinks! If this | ||
| // lint suppression is removed, it autofixes the import path to an invalid | ||
| // package+path entry. | ||
| // eslint-disable-next-line import/no-relative-packages |
There was a problem hiding this comment.
I was wondering if maybe these comments should explain why this is being done since I can imagine something maybe trying to fix this down the road and moving it back to perseus-core. Maybe this PR is enough to prevent that though XD
There was a problem hiding this comment.
I edited these comments to hopefully be clearer. :)
| @@ -5,7 +5,6 @@ | |||
| "rootDir": "src", | |||
| }, | |||
| "references": [ | |||
| {"path": "../perseus-core/tsconfig-build.json"}, | |||
There was a problem hiding this comment.
We didn't need to do this for the other packages that no longer depend on perseus-core?
There was a problem hiding this comment.
I removed this for every Package where we no longer depend on @khanacademy/perseus-core (so each package.json has a matched tsconfig-build.json change. :)
| @@ -0,0 +1 @@ | |||
| ../../../utils/shared | |||
There was a problem hiding this comment.
I think they're symlinks to the utils/shared directory.
My understanding is that he wants to share the logic across packages, but doesn't want the logic to be in a package (to avoid circular dependencies). So each package has a "directory" with the logic, but really the directory is just pointing to a different place.
Summary:
As we are working on Server-Side Scoring, the fact that the
addLibraryVersionToPerseusDebugfunction lived in@khanacdemy/perseus-corewas becoming problematic. All packages need to depend onperseus-corebecause they all need to use this function.This PR moves the function out to a top-level directory and then each package simply imports it using a relative-path import. This violates our lint rules so we suppress it and because we do this, each package will be a few hundred bytes larger. But it completely eliminates the need for cross-package dependencies just for this one function.
Issue: LEMS-2881
Test plan:
I ran
yarn buildand inspected the files in each package'sdist/folder. They had the built version number properly embedded in their output. They also had theaddLibraryVersionToPerseusDebugfunction in them (the fn was copied into the bundle successfully).