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 exports #6521

Merged
merged 1 commit into from
Nov 2, 2022
Merged

fix: cosmic-proto exports #6521

merged 1 commit into from
Nov 2, 2022

Conversation

turadg
Copy link
Member

@turadg turadg commented Nov 1, 2022

Description

https://www.npmjs.com/package/@agoric/cosmic-proto doesn’t have any code. I thought #6510 would solve that, but a build of agoric-sdk with it still doesn’t have them.

I tried removing the swingset/* symlinks but it broke consumers that don't understand package.json exports.

Security Considerations

--

Documentation Considerations

--

Testing Considerations

CI. I see this is covered (transitively) by test-publish-bundle.js.

@turadg turadg requested a review from kriskowal November 1, 2022 21:27
@turadg turadg force-pushed the ta/cosmic-proto-take2 branch from 740ff3b to d62fda5 Compare November 2, 2022 17:44
@@ -38,7 +38,7 @@
"files": [
"build",
"LICENSE*",
"swingset/*"
"gen"
Copy link
Member

Choose a reason for hiding this comment

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

This will work if we include both swingset (symlink sources) and gen (symlink targets).

Comment on lines +18 to +19
"./swingset/msgs.js": "./gen/agoric/swingset/msgs.js",
"./swingset/msgs.ts": "./gen/agoric/swingset/msgs.ts",
Copy link
Member

Choose a reason for hiding this comment

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

This is probably okay but we still need the symlinks for systems that don’t support exports directives, like some bundlers. It would probably be safer in the face of maintenance to leave these alone and rely on the symlinks.

@turadg turadg force-pushed the ta/cosmic-proto-take2 branch from d62fda5 to 36a1f8c Compare November 2, 2022 20:34
@kriskowal kriskowal self-requested a review November 2, 2022 21:20
@turadg turadg added the automerge:no-update (expert!) Automatically merge without updates label Nov 2, 2022
@mergify mergify bot merged commit 06318d5 into master Nov 2, 2022
@mergify mergify bot deleted the ta/cosmic-proto-take2 branch November 2, 2022 23:25
@turadg
Copy link
Member Author

turadg commented Nov 4, 2022

When I npm install this package, the swingset path is absent, even though it's in files. With the exports change I hoped my consuming package would get them from gen but it's not.

I though this might be sanitization in the registry, but I just noticed npm pack locally also omits swingset in its output (even though it includes it in the local tarball):

npm notice === Tarball Contents ===
npm notice 11.4kB  LICENSE
npm notice 184B    README.md
npm notice 17.9kB  gen/agoric/swingset/msgs.js
npm notice 24.9kB  gen/agoric/swingset/msgs.ts
npm notice 65B     gen/gogoproto/gogo.js
npm notice 65B     gen/gogoproto/gogo.ts
npm notice 92.1kB  gen/google/protobuf/descriptor.js
npm notice 102.2kB gen/google/protobuf/descriptor.ts
npm notice 1.5kB   package.json

Apparently this is by design: npm/npm#3310

I'll work on a prepack to include them.

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