-
Notifications
You must be signed in to change notification settings - Fork 59
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
feat: add ts cmd #671
feat: add ts cmd #671
Conversation
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 add some TS guidance to the readme with this? There's nothing in there that covers the support we've added for it, or how to use 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.
Overall this looks good! Just found some minor details that should be fixed
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.
Thanks @hugomrdias this is so great, just one issue that I think that needs to be addressed. Everything else just suggestion or questions.
Iconography of feedback
💭 - Just an idea or an opinion (take it or ignore it as you pleased)
🚨- Problem that should be addressed
❓- Question
cmds/ts.js
Outdated
|
||
\`\`\`json | ||
"types": "./dist/src/index.d.ts", |
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.
❓Why both typeVersions
and types
? Could having both cause some problems ?
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.
types works for the default package export
typeVersions works for atomic internal package exports
cmds/ts.js
Outdated
|
||
\`\`\`json | ||
"types": "./dist/src/index.d.ts", | ||
"typesVersions": { | ||
"*": { "src/*": ["dist/src/*"] } |
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.
🚨We have discovered couple of issues microsoft/TypeScript#41284, microsoft/TypeScript#41281 with this setup and a workaround by doing following:
"*": { "src/*": ["dist/src/*"] } | |
"*": { "src/*": ["dist/src/*", "dist/src/*/index"] } |
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.
❓Is limiting everything to src
deliberate ? And does that not cause problem with package.json
that will be falling outside of src
? Also I think I remember seeing some packages that did not have src
, but might be conflating with some of the newer IPLD stuff that doesn't use aegir.
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.
using src
is an aegir convention
i can't understand the problem with
"types": "./dist/src/index.d.ts",
"typesVersions": {
"*": { "src/*": ["dist/src/*"] }
}
in the issues above
This PR adds support for TS type check, .d.ts generations and docs from .d.ts
closes #568
closes #637