Agoric SDK integration fixes #1889
Conversation
|
Tagged @michaelfig for the review so at least you know about the fix for publish. We should do the same in SDK to ensure dapps get correct types from npm. |
There was a problem hiding this comment.
LGETM -- looks good enough to me.
I will note that the change to importing all types inline at the point of use leads to the type notation being much less usable than before. However, I (almost) understand why this is necessary, and that we have no other reasonable choice.
You should probably still wait for @michaelfig 's review. My approval is just FWIW.
.github/workflows/ci.yml
Outdated
| # Concurrency is 1 to avoid bizarre cross-package contamination. | ||
| run: yarn run lerna exec --concurrency=1 yarn pack | ||
| run: yarn lerna run pack | ||
|
|
||
| - name: clean:types | ||
| # Concurrency is 1 to avoid bizarre cross-package contamination. | ||
| run: yarn lerna run clean:types |
There was a problem hiding this comment.
Please remove # Concurrency is 1... comments, or add more description to explain why it really is 1, and not 10 as I have in my Endo branch.
There was a problem hiding this comment.
Oh, and yarn lerna run pack does absolutely nothing if there is no explicit packageJson.scripts.pack lifecycle script:
$ yarn lerna run pack
yarn run v1.22.19
$ /Users/michael/agoric/endo/node_modules/.bin/lerna run pack
lerna notice cli v5.6.2
lerna info versioning independent
lerna success run No packages found with the lifecycle script 'pack'
✨ Done in 0.76s.
$
Please revert to yarn lerna exec yarn pack or simply just yarn pack.
There was a problem hiding this comment.
Ahah! Thanks for the tip. I’m going to continue using yarn lerna run for build:types and clean:types since ignoring the step on packages that provide no such script is a useful behavior, and I’m reverting to your original lines for pack.
There was a problem hiding this comment.
Also, I don’t understand the concurrency concern. It’s definitely the case that build:types needs to run from the bottom up, but it could do so concurrently. With build:types occurring in a separate step, we could probably safely pack concurrently as well. To that end, I’m going to remove the concurrency limit and comment; please let me know if they remain necessary.
| /// <reference types="ses"/> | ||
|
|
||
| const { quote: q, bare: b, details: X, Fail } = assert; | ||
| const { entries, values } = Object; | ||
| const { ownKeys } = Reflect; | ||
|
|
||
| /** @type {WeakSet<Pattern>} */ | ||
| /** @type {WeakSet<import('./internal-types.js').Pattern>} */ |
There was a problem hiding this comment.
I see you were trying to avoid intrusive changes, but this is too much verbose noise. Can you please rename ./internal-types.js to ./types.js to follow the same filename convention as eventual-send (internal=types.{js,d.ts}, external=exports.{js,d.ts})?
There was a problem hiding this comment.
Sure, I can line this up in this change with @erights’s blessing. It’ll be a separate commit.
There was a problem hiding this comment.
I do not foresee an objection, so I’ve merged for expedience. Do let me know if I should revert the refactor of internal-types.js to merely types.js.
kriskowal
force-pushed
the
kriskowal-sync-agoric-sdk-2023-11
branch
from
aa9c140
to
ea0d31b
Compare
kriskowal
force-pushed
the
kriskowal-sync-agoric-sdk-2023-11
branch
from
ea0d31b
to
b7f18a7
Compare
refs: Agoric/agoric-sdk#8514
Description
Various changes to Endo are necessary to fix integration with Agoric SDK such that Endo packages can be upgraded there.
The largest necessary change is that we must generate the types for each package based on the generated types of all dependencies. To do this, we replace prepack and postpack scripts with build:types and clean:types, then drive them from the top level before publishing.
Then, certain ambient types must be explicitly imported and the
.jsextension is required throughout.We also needed to relax some types in Exo and Patterns.
Security Considerations
Scaling Considerations
Documentation Considerations
Testing Considerations
Upgrade Considerations