Add TypeScript declaration #15

jokester · 2017-04-10T15:41:41Z

This index.d.ts should be sufficient for most library consumers in TypeScript.

There are missing points though. To name a few:

less common but legal option names, like escape-Script
optGet / optSet with numerical option id. They are hard to remember and unstable across versions so I didn't generate overloads for them.

gagern

Thenk you for contributing this proposal. There are a number of items we should discuss before merging this, but I like where this is going and am sure we can work out the details.

gagern · 2017-04-10T20:52:53Z

README.md

+## TypeScript support
+
+This library ships with a `.d.ts` for TypeScript users.
+There is no need to find in `@types/` or roll your own \<3.


Are you willing to maintain the TypeScript support in the future? If not, I guess I'd find a formulation which expresses that this is not an integral part of the codebase, particularly since I'm not sure I can keep it up.

Either way I guess I'd get rid of the <3 eventually, as I prefer a drier style for public documentation and reserve hearts for personal communication. Unless you insist…

I'm fine to maintain the TS support part (likely to use this library in a few more projects).

Feel free to change/remove the readme if you find it inconsistent :)

gagern · 2017-04-10T20:53:40Z

package.json

  "version": "0.3.2",
  "description": "Node bindings to the HTML Tidy library",
  "main": "src/index.js",
+  "types": "src/index.d.ts",


Feel free to also add yourself to the contributors section.

gagern · 2017-04-10T21:10:48Z

src/index.d.ts

+
+/**
+ * TidyOption type and constructor (not for public use)
+ */


If it's not for public use, then better leave this out. I had included it in the API docs for two reasons: to explain that TidyOption has a constructor, which can be used for instanceof checks and so on, and to explicitely state that the constructor should not be used. Not including it in the TS library would strengthen that message I think.

gagern · 2017-04-10T21:18:28Z

src/index.d.ts

+    parseBufferSync(document: Buffer): string
+    runDiagnosticsSync(): string
+    saveBufferSync(): Buffer
+    getErrorLog(): string


The getErrorLog method is currently not documented, because it should not be needed for normal operations. Every error should either be returned from a sync call or passed to the callback of an async call. It's just for the sake of a unit test that I allow access to this, to ensure we're not generating additional errors that we fail to report. The method may disappear at any moment. Perhaps I should rename it to _getErrorLog to communicate that idea.

gagern · 2017-04-10T21:29:09Z

src/index.d.ts

+/************************************************************************
+ * The Generated namespace is generated, and not meant for manual edit.
+ *
+ * @see https://gist.github.com/jokester/5c18601db8a9b290fabcfbab4cfc454b


Better include this script with the codebase. Feel free to create a directory called util and put it there. Bonus points if you can rig up recreation of this portion as part of the prepublish phase.

gagern · 2017-04-10T21:34:07Z

src/index.d.ts

+         * accessibility-check
+         */
+        optGet(key: "accessibility-check" | "accessibility_check" | "AccessibilityCheck"): 0 | 1 | 2 | 3;
+        optSet(key: "accessibility-check" | "accessibility_check" | "AccessibilityCheck", value: 0 | 1 | 2 | 3): void;


The optGet part is incorrect: this version does return a string from the picklist. The optSet part is not exhaustive, as all of these integer-typed pick-value options accept integers or strings. Might be worth modeling.

gagern · 2017-04-10T21:36:54Z

src/index.d.ts

+
+    interface OptionDict {
+        indent_spaces?: number;
+        IndentSpaces?: number;


If you specify the dict like this, people may conceivably pass conflicting options to the constructor. I'd rather use a white lie and omit all the camel-cased names. That helps keep the sources consistent and avoids conflicts.

gagern · 2017-04-10T21:38:22Z

test/test-decl.ts

+ *
+ * required package (not included in package.json):
+ * - ts-node or tsc
+ * - @types/chai


Feel free to add these as dev dependencies, and to include these tests in the npm test machinery.

gagern · 2017-04-10T21:52:00Z

Oh, and one more thing: can you please conform to the two spaces of indentation I'm using?

jokester · 2017-04-12T02:22:44Z

instanceof still works

optGet: returns a string from picklist optSet: accepts index in picklist as well

jokester · 2017-04-14T01:52:43Z

@gagern Pushed a few more commits. Mind checking them?

The generation script is included but not added to prepublish. I guess it safer to run update-tsd and have a human check it. Feel free to contact me when needed.

jokester · 2017-04-24T13:12:10Z

@gagern Haven't hear from you recently, but .. do you think there are something we should do before merging this?

gagern

Sorry for the delay. Still some questions left.

gagern · 2017-04-23T22:02:31Z

package.json

  "devDependencies": {
+    "@types/chai": "^3.5.0",
+    "@types/mocha": "^2.2.40",
+    "@types/node": "^7.0.12",


I wonder whether we should try to match the versions of these scoped packages here to those of the corresponding unscoped packages below. Any reasons in favor or against doing so?

This one is a bit tricky, @types/ packages does not cover some of corresponding JS packages. Will see if I can fix or remove some packages.

I was mostly referring to those which have unscoped counterparts. No need to change the set of files, just the versions.

Sorry to say this, but the @types/mocha package does not have a 3.x version. The version 2.2 still it covers the mocha API we need.
Should I use this mismatched version, or remove this package and declare a minimal mocha API in place?

In that case better use the mismatched version. As far as I can tell, the TypeScript dependencies don't pull in the actual libraries, so we shouldn't see much impact from the mismatch unless we start using API which is different between those versions. Right?

A Pull Request against the DefinitelyTyped might be a good idea if you want to, but I think we don't block on that here.

gagern · 2017-04-23T22:04:04Z

util/tsconfig.json

+    "atom": {
+        "rewriteTsconfig": false
+    }
+}


Do we really need all of these settings? I'd prefer going with defaults as much as possible unless there is some major downside to doing so, in order to keep the number of settings that may need to be maintained low.

It seems we can remove that file and still run the .ts code. Guess I should remove it.

👍 to that.

gagern · 2017-04-24T21:09:20Z

README.md

+
+This library ships with a `.d.ts` declaration for TypeScript users.
+
+For contributors: the declaration is updated with the script in `util/gen-typescript-decl.ts`.


Or easier: “… updated by running npm run update-tsd”.

gagern · 2017-04-24T21:24:44Z

src/index.d.ts

+    * PrettyPrint / indent-spaces (integer)
+    */
+    optGet(key: "indent-spaces" | "indent_spaces" | "IndentSpaces"): number;
+    optSet(key: "indent-spaces" | "indent_spaces" | "IndentSpaces", value: number): void;


Does this level of detail for the getters and setters really make sense? It does sound reasonable if you really are configuring these things manually, using string literals as keys. But those applications are easier to write using the options dictionary notation, I think. What happens if the configuration comes from a JSON file or similar? Is there some easy solution to make that work, too, or would that mean you'd have to type-cast the key from the JSON file to the enum of all the alternatives you have configured here? Would it make sense to add a catch-all definition up front? For the getter, that would be easy: optGet(key: string): number | string | boolean I think. For the setter that would become optSet(key: string, value: number | string | boolean) which would defy all the special cases, as this one case would capture anything not captured by the special cases, thus breaking the type checking there. So should we only do the specific getters, and the generic getter and setter? I'd like to see a generic get and set (using separately typed variables for key and value) demonstrated in the unit test.

A value from JSON should still work without much effort: the TypeScript compiler (as of now) considers such a value to have any type, and excludes it from type check.

This is more of a limitation, those who read configure from JSON file cannot benefit from type checker.

If I may say, adding generic getter / setter is not my favorite idea, mostly because allowing potentially incorrect option key is somehow against type checking. Can I ask what are some special cases that requires generic getter / setter?

Correct me if I'm wrong, but here is what I imagine: When you read from JSON, the value would be of type any, but the key would be of type string, with no restriction as to what kind of string it would be. So you can't use the key from the JSON to configure these, unless you cast the string to something more generic. Right? I originally assumed you'd have to cast to the full list of enum values, but probably casting to any would be the more practical solution.

Another scenario might be query string parameters, which map string to string. As libtidy can parse strings, that's perfectly fine in terms of types, but would be problematic in a type-checked TypeScript.

Guess I got your idea, but this one didn't go quite well. It seems that when optGet has a generic overload, its return type will always be number | string | boolean regardless of the callsite, for example:

// Type 'string | number | boolean' is not assignable to type 'boolean'. const markup: boolean = doc.optGet("markup");

(see this commit for detail)

Do you think we can go with specialized getters and generic setters?

Sorry, lost track of this PR. Is there any reason why you'd write the above in that way, instead of

const markup: boolean = doc.options.markup;

That way you could use the dict for specific access, and use the getters for generic access, i.e. things like this:

for (let o: doc.getOptionList()) console.log({[o.name]: doc.optGet(o.name)})

Likewise, having all the various forms for values in the dictionary is harmful since you can't write things like

const check: string = doc.opts.accessibility_check;

which I consider bad. Anyway, I finally got round to merging your PR, and did clean up the interface according to my taste in 0dc12f4.

gagern: guess I did not consider that case. It looks much better with your fix, thanks!

gagern · 2017-04-24T21:30:03Z

util/gen-typescript-decl.ts

+// The rest of this file is generated.
+` + generate();
+
+  if (newContent === oldContent) {


Can you add a command line flag for “verify mode”, where you compare the content and exit with an error message and a non-zero exit status if there is a mismatch? Then we can have Travis run that mode to ensure the generated file stays in sync with the commited changes to the core library. This becomes moot if we autogenerate the code upon presubmit and leave it out of the repository.

I'm getting a little confused here: if we output the generated code to a separated file, and do not include it in the repo, is a "verify mode" still needed?

True. That's what I meant with by “this becomes moot”.

Understood. Guess I should add the generation script to pretest hook as well?

Sounds good, yes.

gagern · 2017-04-24T21:31:32Z

src/index.d.ts

+  }
+}
+
+// START-GENERATED-NAMESPACE


Can you move this generated code to a separate file? I still believe not having the generated code in the repository, but generating it upon publication, would be the cleaner solution. That way we would not have to worry about inconsistent state, as there would be only a single source of truth, namely the tidy library. And with two files, it's easy to have one in the repository while ignoring the other. Automatically patching files is far more tricky in this respect.

gagern · 2017-05-01T23:51:58Z

Looks like a chicken and egg problem: you can't run the generator to build the options unless you already have the options build by the generator. One solution would be having the generator written in plain JavaScript. Another would be handling the option verification somewhere else, e.g. in a unit test.

jokester · 2017-05-02T01:29:31Z

Thanks for pointing out. I was able to reproduce the chicken-egg problem in VS Code editor (strangely, not when running the script). Guess changing the script to JavaScript is the safer fix.

Add TypeScript declaration

gagern requested changes Apr 10, 2017

View reviewed changes

jokester added 13 commits April 14, 2017 09:07

add typescript declaration

79dd36d

mention TypeScript support

56bd127

fix: add options

63683e0

copy test cases from doc-test

cb827a7

fix link to decl-generate script

387a2b0

include the script to generate .d.ts

1ac203d

run test/ts-decl-test.ts with mocha

0631cb2

add me to contributors shyly

ed2630e

add npm script "update-tsd"

c22a539

remove camel-cased names from OptionDict

1cf3c10

prevent library consumer from creating TidyOption

9feb2ab

instanceof still works

change indent to 2 spaces

01ccf71

make README drier

e0ad4db

jokester force-pushed the add-ts-decl branch from 58f7cff to e0ad4db Compare April 14, 2017 00:09

jokester added 5 commits April 14, 2017 10:42

hide unneeded getErrorLog()

9ce1ae6

shortify comment

8caab60

add category/type in tsdoc

5ab18c2

fix return of optGet / param of optSet

750dd6c

optGet: returns a string from picklist optSet: accepts index in picklist as well

note a limitation of TS declaration

5806f74

jokester force-pushed the add-ts-decl branch from 8849202 to 5806f74 Compare April 14, 2017 01:42

gagern reviewed Apr 24, 2017

View reviewed changes

jokester added 3 commits May 1, 2017 21:03

split generated code to options.d.ts

aad6107

remove unnecessary TypeScript config / deps

858d3a4

generate src/options.d.ts on the fly

a5ccfc3

jokester added 3 commits May 2, 2017 11:43

change generator to JavaScript

2b29311

remove ... operator for node v4

c02558c

update README

2f45165

gagern merged commit 2f45165 into gagern:master Aug 6, 2017

gagern added a commit that referenced this pull request Aug 6, 2017

Merge pull request #15 from jokester/add-ts-decl

0960951

Add TypeScript declaration


		This library ships with a `.d.ts` declaration for TypeScript users.

		For contributors: the declaration is updated with the script in `util/gen-typescript-decl.ts`.

Add TypeScript declaration #15

Add TypeScript declaration #15

Uh oh!

Conversation

jokester commented Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gagern left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gagern commented Apr 10, 2017

Uh oh!

jokester commented Apr 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jokester commented Apr 14, 2017

Uh oh!

jokester commented Apr 24, 2017

Uh oh!

gagern left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jokester Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jokester commented Apr 10, 2017 •

edited

Loading

jokester commented Apr 12, 2017 •

edited

Loading

jokester Apr 28, 2017 •

edited

Loading