Closed davidchambers closed 5 years ago
Thanks for the quick feedback, @bcherny. :)
Why are you trying so hard to avoid a dev time dependency?
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 rollup@0.60.7 rollup-plugin-typescript@0.8.1 typescript@2.9.2
$ 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 total
Although 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.
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 import
/export
. Given that TypeScript files must necessarily be processed before being evaluated, is there a reason not to use export
and const
in that context?
@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 #!/bin/sh
POSIX compatible? As far as I can tell they don't use any bashisms.
I'm no shell scripting expert, but, is there any reason for not making the scripts
#!/bin/sh
POSIX compatible? As far as I can tell they don't use any bashisms.
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 #!/usr/bin/env bash
will not cause us any problems.
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
awk
script is just a single line:Unlike #270, this pull request uses string literal types in index.d.ts rather than the far less specific
string
type. 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 lint
such that ignoring these instructions will result in an error.