-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
WIP Playing around with .d.ts generation #4156
Conversation
package-scripts.js
Outdated
description: 'Build' | ||
}, | ||
tsc: { | ||
script: 'tsc -p . || 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.
FYI, you don't need to compile with TS. You can set noEmit: true
or emitDeclarationsOnly: true
and avoid transpiling the JS (invoking the compiler will then just typecheck/emit declarations). I imagine that's better for keeping the project simpler?
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.
Yeah, I wanted to give it a try just to see if everything still works, but you're right. I'll make it clearer that we don't need to do this.
@@ -29,7 +29,10 @@ var assign = (exports.assign = require('object.assign').getPolyfill()); | |||
* @throws {TypeError} if either constructor is null, or if super constructor | |||
* lacks a prototype. | |||
*/ | |||
exports.inherits = util.inherits; | |||
exports.inherits = function(ctor, superCtor) { | |||
// Wrapper prevents type declarations from referring to @types/node |
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.
You could also just add a /** @type {whatever} */
annotation onto this declaration to override the inferred type, instead of changing the implementation.
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.
@cspotcode I like where this is going, thanks. I haven't had a chance to try it out yet, but I think this is on the right track.
Have you gotten a chance to build the API docs against it? One of my concerns was that our API doc generation might become wonky if we mess with existing docstrings too much, but I don't think we're yet at "too much". I would like to see what happens with an import(..)
type though
package?: boolean; | ||
/** Filepath to mocha.opts; defaults to whatever's in `mocharc.opts`, or `false` to skip */ | ||
opts?: string | false; | ||
} |
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.
please do this with a @typedef
; I really don't want to add any typescript sources.
needs conflicts to be resolved |
I like this idea, I think this is something to move forward with |
👋 coming back to this @cspotcode, are you still interested in working on this PR? As of #5027 there are again maintainers who can review it now. No worries if you no longer have time - but if you do that'd be great! |
Related to #4154
EDIT: got rid of the stuff about compiling; switched to
emitDeclarationsOnly: true
.To play around with it locally,
yarn nps build.types
andyarn nps test.types