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

fix cosmic-proto swingset exports #6540

Merged
merged 4 commits into from
Nov 10, 2022
Merged

fix cosmic-proto swingset exports #6540

merged 4 commits into from
Nov 10, 2022

Conversation

turadg
Copy link
Member

@turadg turadg commented Nov 4, 2022

Description

As described in #6521 (comment)

Third time's a charm!

Security Considerations

Documentation Considerations

explained in README

Testing Considerations

❯ npm pack

> @agoric/cosmic-proto@0.2.1 prepack
> yarn rebuild

yarn run v1.22.19
$ rm -rf gen && mkdir -p gen && yarn generate && tsc --build && yarn prettier -w swingset
$ protoc --plugin=node_modules/.bin/protoc-gen-ts_proto --ts_proto_opt='esModuleInterop=true,forceLong=long,useOptionals=messages' --ts_proto_out=./gen --ts_proto_opt=importSuffix=.js ./proto/agoric/swingset/msgs.proto ./proto/agoric/swingset/query.proto -I./proto
$ /opt/agoric/agoric-sdk/node_modules/.bin/prettier -w swingset
swingset/msgs.d.ts 243ms
swingset/msgs.js 96ms
swingset/query.d.ts 39ms
swingset/query.js 20ms
swingset/swingset.d.ts 35ms
swingset/swingset.js 35ms
✨  Done in 6.00s.
npm notice
npm notice 📦  @agoric/cosmic-proto@0.2.1
npm notice === Tarball Contents ===
npm notice 11.4kB  LICENSE
npm notice 352B    README.md
npm notice 17.7kB  dist/agoric/swingset/msgs.d.ts
npm notice 17.9kB  dist/agoric/swingset/msgs.js
npm notice 14.2kB  dist/agoric/swingset/query.d.ts
npm notice 9.5kB   dist/agoric/swingset/query.js
npm notice 16.3kB  dist/agoric/swingset/swingset.d.ts
npm notice 16.9kB  dist/agoric/swingset/swingset.js
npm notice 3.3kB   dist/cosmos/base/v1beta1/coin.d.ts
npm notice 6.2kB   dist/cosmos/base/v1beta1/coin.js
npm notice 52B     dist/gogoproto/gogo.d.ts
npm notice 65B     dist/gogoproto/gogo.js
npm notice 53B     dist/google/api/annotations.d.ts
npm notice 66B     dist/google/api/annotations.js
npm notice 83.9kB  dist/google/api/http.d.ts
npm notice 9.0kB   dist/google/api/http.js
npm notice 4.6MB   dist/google/protobuf/descriptor.d.ts
npm notice 104.5kB dist/google/protobuf/descriptor.js
npm notice 1.8kB   package.json
npm notice === Tarball Details ===
npm notice name:          @agoric/cosmic-proto
npm notice version:       0.2.1
npm notice filename:      @agoric/cosmic-proto-0.2.1.tgz
npm notice package size:  134.0 kB
npm notice unpacked size: 5.0 MB
npm notice shasum:        9e1e9c84709280241e06a4d27cfc0b8ba5536df4
npm notice integrity:     sha512-Z9LI2qzsVwnTF[...]0Ndf7WA1Zxm5A==
npm notice total files:   19
npm notice

@turadg turadg requested a review from kriskowal November 4, 2022 15:35
Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

A bit of a shame to have build products checked in.

I’m surprised no further ceremony was necessary to mute the useOptional=messages warnings. When I tried that, I ran into some problem or another. It looked like it produced a breaking change in the generated code.

@turadg turadg force-pushed the ta/fix-proto-symlink branch 2 times, most recently from 1d8b97b to ad84094 Compare November 4, 2022 22:34
@turadg
Copy link
Member Author

turadg commented Nov 4, 2022

While looking into this more I realized what I submitted wouldn't solve my downtream problem because it has .ts files which will run into microsoft/TypeScript#47387 (comment)

The solution is for NPM packages to distribute only .js and .d.ts files. So I made some changes here to do that. It has a dist directory that the .d.ts and .js get build to, from the .ts in gen. gen doesn't need to stay in CI because it's just an interim build artifact. By some ideal dist wouldn't either but by putting it in we make it so agoric-sdk consumers don't have to depend on the build state of this package.

@turadg turadg marked this pull request as draft November 7, 2022 19:22
@turadg turadg force-pushed the ta/fix-proto-symlink branch 2 times, most recently from e9e44d2 to b70d063 Compare November 8, 2022 22:35
@turadg turadg marked this pull request as ready for review November 8, 2022 22:35
@turadg turadg requested a review from kriskowal November 9, 2022 20:11
Comment on lines 19 to 30
"./swingset/msgs.js": {
"types": "./dist/agoric/swingset/msgs.d.ts",
"default": "./dist/agoric/swingset/msgs.js"
},
"./swingset/query.js": {
"types": "./dist/agoric/swingset/query.d.ts",
"default": "./dist/agoric/swingset/query.js"
},
"./swingset/swingset.js": {
"types": "./dist/agoric/swingset/swingset.d.ts",
"default": "./dist/agoric/swingset/swingset.js"
}
Copy link
Member

Choose a reason for hiding this comment

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

Could we reduce duplication by exporting the copy in swingset and not include dist in the published artifact? Multiple physical locations could effect multiple module instances and an identity discontinuity between them in the same program.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call. We don't really need dist in the source tree. Force-pushed this change and will merge if it passes CI.

@turadg turadg added the automerge:no-update (expert!) Automatically merge without updates label Nov 9, 2022
},
"files": [
"build",
"dist",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"dist",

Copy link
Member Author

Choose a reason for hiding this comment

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

well I spoke too soon. integrating with master, swingset now depends on cosmos which is at a higher level such that copying it to root would require moving swingset. I had to revert to the symlink approach.

So all this does now is add dist for consumers of the package through NPM. Keeps symlinks for peer packages in the repo.

@turadg turadg removed the automerge:no-update (expert!) Automatically merge without updates label Nov 10, 2022
Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

Whatever works.

@turadg turadg added the automerge:no-update (expert!) Automatically merge without updates label Nov 10, 2022
@mergify mergify bot merged commit 432aecb into master Nov 10, 2022
@mergify mergify bot deleted the ta/fix-proto-symlink branch November 10, 2022 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:no-update (expert!) Automatically merge without updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants