-
Notifications
You must be signed in to change notification settings - Fork 2
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 ESM dist for npm package #102
Conversation
"type": "module", | ||
"module": "./dist/lib/index.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add an exports
field to prevent external users doing deep imports into dist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was reluctant to make that big a change yet, but I suppose consumers that it would break just won't upgrade.
However, it's for an additional improvement over the status quo. While it's good, it's not necessary to unblock the issue you raised. Can you wait for this to be future work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it'll break consumers already with the move, might as well make it official. Introducing exports
is technically semver major, might as well bite the bullet once
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the move / introduction of a dist
folder, I would strongly prefer if the package.json added an exports
field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's sync on supporting tests like Agoric/agoric-sdk#8786 and Agoric/agoric-sdk#8787
8a506a4
to
d11fa6a
Compare
entry: ['src/cli/cli.ts', 'src/lib/index.ts'], | ||
format: ['esm'], | ||
dts: true, // Generate declaration file (.d.ts) | ||
sourcemap: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we generate d.ts.map
as well? Going to implementation is broken in VS Code without it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh apparently it's a limitation: egoist/tsup#564
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it shouldn't be necessary here, I'm confused why go to definition is not working for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have confirmed this works in an agoric-sdk branch!
ce078d9
to
6b18e29
Compare
6b18e29
to
da6f0aa
Compare
A recent conversion of a lib file to
.ts
broke downstream deep imports of it.This builds
.js
files in the distribution and prevents deep imports.