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: automatically build types in prepack #2258

Merged
merged 3 commits into from May 21, 2024
Merged

build: automatically build types in prepack #2258

merged 3 commits into from May 21, 2024

Conversation

turadg
Copy link
Member

@turadg turadg commented Apr 29, 2024

refs: #1488

Description

Simplify build by using NPM lifecycle hooks. Adds a CI test for the package graph sensitivity that the extant method was checking.

Part of making it work was adding "typescript" to devDependencies of packages that need it. The root build:types was running with the root tsc but when building in each package separately they wouldn't get the root's typescript version and fall back to the global one, which didn't know the @import syntax.

Security Considerations

None

Scaling Considerations

None

Documentation Considerations

Less to document

Testing Considerations

I tried the "publish" commands in CONTRIBUTING.md.

integration PR in agoric-sdk: Agoric/agoric-sdk#9385

Compatibility Considerations

Removes a necessary accommodation on agoric-sdk

Upgrade Considerations

None

@turadg turadg mentioned this pull request Apr 29, 2024
1 task
@turadg turadg marked this pull request as ready for review May 20, 2024 01:26
@turadg turadg requested a review from kriskowal May 20, 2024 01:26
@turadg turadg force-pushed the ta/prepack-types branch 2 times, most recently from 5423a2c to a83af07 Compare May 20, 2024 18:12
"build:types": "tsc --build tsconfig.build.json",
"clean:types": "git clean -f '*.d.ts*'",
"prepack": "tsc --build tsconfig.build.json",
"postpack": "git clean -f '*.d.ts*'",
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a reason to believe that cleaning up types in postpack will work now when it did not before? #1864

Copy link
Member Author

Choose a reason for hiding this comment

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

Many things have changed since then. Named exports and imports for one.

To be sure, can you describe how I could reproduce the problem described in that PR intro? I'd like to try making a regression test, something that failed right before 1864 and passed right after.

Copy link
Member

Choose a reason for hiding this comment

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

Integration with Agoric SDK satisfies me. Hopefully it works this time because this is by far the better state.

Comment on lines +330 to +331
- name: Prepack packages
run: yarn lerna run --reject-cycles --concurrency 1 prepack
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what the purpose is to generate types after the build is created?

Copy link
Member Author

Choose a reason for hiding this comment

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

Each package is built with access to the entire monorepo source and without other typedefs. When packages are imported, they'll have only the package dependencies and those will have typedefs instead of original src. I've seen errors in resolving at that point.

@turadg turadg merged commit 0a91fbe into master May 21, 2024
17 checks passed
@turadg turadg deleted the ta/prepack-types branch May 21, 2024 18:26
@mhofman
Copy link
Contributor

mhofman commented May 21, 2024

FYI, running prepack in topo order vs reverse topo order (to simulate deps not being built before) yields in the following diff.
From what I can tell the differences while not obvious, are benign. This should confirm that building the types of packages independently is now working.

diff --git topo/packages/marshal/src/encodeToSmallcaps.d.ts rev-topo/packages/marshal/src/encodeToSmallcaps.d.ts
index ebb499146..ede8c1d13 100644
--- topo/packages/marshal/src/encodeToSmallcaps.d.ts
+++ rev-topo/packages/marshal/src/encodeToSmallcaps.d.ts
@@ -3,15 +3,14 @@ export function makeDecodeFromSmallcaps(decodeOptions?: DecodeFromSmallcapsOptio
 export type SmallcapsEncoding = any;
 export type SmallcapsEncodingUnion = any;
 export type EncodeToSmallcapsOptions = {
-    encodeRemotableToSmallcaps?: ((remotable: typeof Remotable, encodeRecur: (p: Passable) => SmallcapsEncoding) => SmallcapsEncoding) | undefined;
+    encodeRemotableToSmallcaps?: ((remotable: <T extends {}, I extends string>(iface?: I | undefined, props?: undefined, remotable?: T | undefined) => T & import("@endo/pass-style").RemotableObject<I> & import("@endo/eventual-send").RemotableBrand<{}, T>, encodeRecur: (p: Passable) => SmallcapsEncoding) => SmallcapsEncoding) | undefined;
     encodePromiseToSmallcaps?: ((promise: Promise<any>, encodeRecur: (p: Passable) => SmallcapsEncoding) => SmallcapsEncoding) | undefined;
     encodeErrorToSmallcaps?: ((error: Error, encodeRecur: (p: Passable) => SmallcapsEncoding) => SmallcapsEncoding) | undefined;
 };
 export type DecodeFromSmallcapsOptions = {
-    decodeRemotableFromSmallcaps?: ((encodedRemotable: SmallcapsEncoding, decodeRecur: (e: SmallcapsEncoding) => Passable) => typeof Remotable) | undefined;
+    decodeRemotableFromSmallcaps?: ((encodedRemotable: SmallcapsEncoding, decodeRecur: (e: SmallcapsEncoding) => Passable) => <T extends {}, I extends string>(iface?: I | undefined, props?: undefined, remotable?: T | undefined) => T & import("@endo/pass-style").RemotableObject<I> & import("@endo/eventual-send").RemotableBrand<{}, T>) | undefined;
     decodePromiseFromSmallcaps?: ((encodedPromise: SmallcapsEncoding, decodeRecur: (e: SmallcapsEncoding) => Passable) => Promise<any>) | undefined;
     decodeErrorFromSmallcaps?: ((encodedError: SmallcapsEncoding, decodeRecur: (e: SmallcapsEncoding) => Passable) => Error) | undefined;
 };
 import type { Passable } from '@endo/pass-style';
-import type { Remotable } from '@endo/pass-style';
 //# sourceMappingURL=encodeToSmallcaps.d.ts.map
\ No newline at end of file
diff --git topo/packages/marshal/src/encodeToSmallcaps.d.ts.map rev-topo/packages/marshal/src/encodeToSmallcaps.d.ts.map
index 541c3835e..bab18f46f 100644
--- topo/packages/marshal/src/encodeToSmallcaps.d.ts.map
+++ rev-topo/packages/marshal/src/encodeToSmallcaps.d.ts.map
@@ -1 +1 @@
-{"version":3,"file":"encodeToSmallcaps.d.ts","sourceRoot":"","sources":["encodeToSmallcaps.js"],"names":[],"mappings":"AAsHO,qHAF8B,iBAAiB,CAgLrD;AA8BM,2GAFgB,iBAAiB,cAqJvC;gCA9ba,GAAG;qCACH,GAAG;;8FAmEkB,iBAAiB,KAC5C,iBAAiB;sFAGU,iBAAiB,KAC5C,iBAAiB;sCAEb,KAAK,gCACkB,iBAAiB,KAC5C,iBAAiB;;;uDAsMF,iBAAiB,mBAClB,iBAAiB;mDAGlB,iBAAiB,mBAChB,iBAAiB;+CAGpB,iBAAiB,mBACd,iBAAiB,kBAC/B,KAAK;;8BA/R0B,kBAAkB;+BAAlB,kBAAkB"}
\ No newline at end of file
+{"version":3,"file":"encodeToSmallcaps.d.ts","sourceRoot":"","sources":["encodeToSmallcaps.js"],"names":[],"mappings":"AAsHO,qHAF8B,iBAAiB,CAgLrD;AA8BM,2GAFgB,iBAAiB,cAqJvC;gCA9ba,GAAG;qCACH,GAAG;;8RAmEkB,iBAAiB,KAC5C,iBAAiB;sFAGU,iBAAiB,KAC5C,iBAAiB;sCAEb,KAAK,gCACkB,iBAAiB,KAC5C,iBAAiB;;;uDAsMF,iBAAiB,mBAClB,iBAAiB;mDAGlB,iBAAiB,mBAChB,iBAAiB;+CAGpB,iBAAiB,mBACd,iBAAiB,kBAC/B,KAAK;;8BA/R0B,kBAAkB"}
\ No newline at end of file
diff --git topo/packages/marshal/src/rankOrder.d.ts rev-topo/packages/marshal/src/rankOrder.d.ts
index 9ce047e6a..0ae719af4 100644
--- topo/packages/marshal/src/rankOrder.d.ts
+++ rev-topo/packages/marshal/src/rankOrder.d.ts
@@ -1,4 +1,4 @@
-export function trivialComparator(left: any, right: any): 1 | -1 | 0;
+export function trivialComparator(left: any, right: any): 0 | 1 | -1;
 export function getPassStyleCover(passStyle: PassStyle): RankCover;
 export function makeComparatorKit(compareRemotables?: RankCompare | undefined): RankComparatorKit;
 export function comparatorMirrorImage(comparator: RankCompare): RankCompare | undefined;
@@ -48,27 +48,27 @@ export type PassStyleRanksRecord = {
         index: number;
         cover: RankCover;
     };
-    tagged: {
+    null: {
         index: number;
         cover: RankCover;
     };
-    remotable: {
+    error: {
         index: number;
         cover: RankCover;
     };
-    null: {
+    copyRecord: {
         index: number;
         cover: RankCover;
     };
-    copyRecord: {
+    copyArray: {
         index: number;
         cover: RankCover;
     };
-    copyArray: {
+    tagged: {
         index: number;
         cover: RankCover;
     };
-    error: {
+    remotable: {
         index: number;
         cover: RankCover;
     };
diff --git topo/packages/pass-style/src/passStyleOf.d.ts rev-topo/packages/pass-style/src/passStyleOf.d.ts
index 6f0658fd6..5fd3dce02 100644
--- topo/packages/pass-style/src/passStyleOf.d.ts
+++ rev-topo/packages/pass-style/src/passStyleOf.d.ts
@@ -19,7 +19,7 @@ export function assertPassable(val: any): void;
 export function isPassable(specimen: any): specimen is Passable;
 export function toPassableError(err: Error): Error;
 export function toThrowable(specimen: unknown): Passable<never, Error>;
-export type HelperPassStyle = "copyRecord" | "copyArray" | "tagged" | "remotable" | "error";
+export type HelperPassStyle = "error" | "copyRecord" | "copyArray" | "tagged" | "remotable";
 import type { PassStyleOf } from './types.js';
 import type { Passable } from './types.js';
 //# sourceMappingURL=passStyleOf.d.ts.map
\ No newline at end of file
diff --git topo/packages/patterns/src/types.d.ts rev-topo/packages/patterns/src/types.d.ts
index 2ab291977..cf22322d9 100644
--- topo/packages/patterns/src/types.d.ts
+++ rev-topo/packages/patterns/src/types.d.ts
@@ -173,7 +173,7 @@ export type Limits = Partial<AllLimits>;
  * PassStyle is 'tagged', then the `getTag` value for tags that are
  * recognized at the
  */
-export type Kind = "remotable" | import("@endo/pass-style").PrimitiveStyle | "copyRecord" | "copyArray" | "error" | "promise" | "copySet" | "copyBag" | "copyMap" | `match:${any}` | `guard:${any}`;
+export type Kind = "error" | import("@endo/pass-style").PrimitiveStyle | "copyRecord" | "copyArray" | "remotable" | "promise" | "copySet" | "copyBag" | "copyMap" | `match:${any}` | `guard:${any}`;
 export type PatternMatchers = {
     /**
      * Matches any Passable.

For reference, the commands used:

yarn lerna ls --all --toposort | cut -d' ' -f1 | tac | xargs -I % yarn lerna run --scope % prepack
git add . && git stash -m "reverse toposort"
yarn lerna run --reject-cycles --concurrency 1 prepack
git add . && git stash -m "run prepack"
git diff --src-prefix=topo/ --dst-prefix=rev-topo/ stash@{0}^2..stash@{1}^2

@kriskowal
Copy link
Member

Wow, brilliant experiment.

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

3 participants