-
Notifications
You must be signed in to change notification settings - Fork 378
derive index.js and index.d.ts from canonical list of method names #294
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
Conversation
bcherny
left a comment
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.
@davidchambers As I said in the other PR, this approach is strange to me. Why not just use tsc the way it’s supposed to be used? Why are you trying so hard to avoid a dev time dependency?
|
Thanks for the quick feedback, @bcherny. :)
It feels like overkill to me. I had a look to see just how much JavaScript code would be involved in generating the type definitions the right way: $ npm install [email protected] [email protected] [email protected]
$ find node_modules -name '*.js' -print0 | xargs -0 wc -l
66 node_modules/estree-walker/dist/estree-walker.umd.js
58 node_modules/estree-walker/dist/estree-walker.es6.js
53 node_modules/estree-walker/src/estree-walker.js
59 node_modules/balanced-match/index.js
223 node_modules/rollup-pluginutils/dist/pluginutils.cjs.js
216 node_modules/rollup-pluginutils/dist/pluginutils.es6.js
4 node_modules/rollup-pluginutils/src/index.js
5 node_modules/rollup-pluginutils/src/utils/ensureArray.js
15 node_modules/rollup-pluginutils/src/makeLegalIdentifier.js
147 node_modules/rollup-pluginutils/src/attachScopes.js
26 node_modules/rollup-pluginutils/src/createFilter.js
14 node_modules/rollup-pluginutils/src/addExtension.js
390 node_modules/tippex/dist/tippex.umd.js
377 node_modules/tippex/dist/tippex.es6.js
17 node_modules/tippex/src/getLocation.js
353 node_modules/tippex/src/index.js
55 node_modules/compare-versions/test/gt.js
59 node_modules/compare-versions/test/sort.js
53 node_modules/compare-versions/test/compare.js
47 node_modules/compare-versions/index.js
313 node_modules/rollup-plugin-typescript/dist/rollup-plugin-typescript.es.js
316 node_modules/rollup-plugin-typescript/dist/rollup-plugin-typescript.cjs.js
54813 node_modules/rollup-plugin-typescript/node_modules/typescript/lib/typescript.js
54813 node_modules/rollup-plugin-typescript/node_modules/typescript/lib/typescriptServices.js
48553 node_modules/rollup-plugin-typescript/node_modules/typescript/lib/tsserver.js
34877 node_modules/rollup-plugin-typescript/node_modules/typescript/lib/tsc.js
18 node_modules/rollup-plugin-typescript/src/resolveHost.js
68 node_modules/rollup-plugin-typescript/src/options.js
153 node_modules/rollup-plugin-typescript/src/index.js
78 node_modules/rollup-plugin-typescript/src/fixExportClass.js
3 node_modules/rollup-plugin-typescript/src/string.js
41 node_modules/rollup-plugin-typescript/src/typescript-helpers.js
117014 node_modules/typescript/lib/tsserverlibrary.js
71 node_modules/typescript/lib/cancellationToken.js
110009 node_modules/typescript/lib/typescript.js
110009 node_modules/typescript/lib/typescriptServices.js
99149 node_modules/typescript/lib/tsserver.js
69132 node_modules/typescript/lib/tsc.js
21246 node_modules/typescript/lib/typingsInstaller.js
27 node_modules/typescript/lib/watchGuard.js
201 node_modules/brace-expansion/index.js
923 node_modules/minimatch/minimatch.js
25776 node_modules/rollup/dist/rollup.es.js
21926 node_modules/rollup/dist/rollup.browser.js
25787 node_modules/rollup/dist/rollup.js
39 node_modules/concat-map/test/map.js
6 node_modules/concat-map/example/map.js
13 node_modules/concat-map/index.js
90 node_modules/object-assign/index.js
797701 totalAlthough I have not been much involved in the development of the Fantasy Land specification, I'm currently actively involved in the running of the project. Any problem with a dependency is likely to be mine to solve. Every dependency is a liability; in this case I do not see sufficient compensation. If the low-tech build pipeline introduced in this pull request causes us problems in the future, I'll be on the hook. I accept this responsibility. Setting aside how the type definitions are generated, are you happy with them? Does this pull request meet the needs of TypeScript users? Is it equivalent to #266 from the perspective of the end user? |
|
Looks good to me. The types are correct and given how simple they are generating them in the proposed manner seems reasonable. |
|
@davidchambers Sounds good. I’m not sure I’d make the same decision, but thanks for taking the time to clarify! TS looks good to me. One note- it looks like the generated JS use ES5-style exports, but the generated TS uses ES6-style exports. If I’m reading that right, I’d suggest sticking to one or the other. |
Restricting the JavaScript file to ES5 syntax enables it to be used as is in environments (such as Node) which don't support |
|
@davidchambers It's just another small deviation from standard TS practice (declare in TS + ES6, compile to JS + ES5 with tsc). Looks good to me! |
|
I'm no shell scripting expert, but, is there any reason for not making the scripts |
93a8a9b to
e07e8d1
Compare
e07e8d1 to
ee102dc
Compare
I like to understand the code I write, but with shell scripts I'm not averse to cargo cult programming. ;) This pull request has been open for a year and a half. Let's merge it. I would not oppose a pull request to change the shebangs, but I'm confident |
This pull request supersedes #270, which included a few unrelated changes which have since been rendered unnecessary (#293).
Like #270, this pull request avoids npm dependencies by using standard Unix programs to generate index.d.ts. The
awkscript is just a single line:Unlike #270, this pull request uses string literal types in index.d.ts rather than the far less specific
stringtype. I'm not a TypeScript user so I would greatly appreciate a review by @bcherny, @andnp, @sunderhus, or anyone else in the community with TypeScript knowledge. :)This pull request also updates CONTRIBUTING.md with instructions for updating the various files, and updates the behaviour of
npm run lintsuch that ignoring these instructions will result in an error.