Update VFile #19583

Rokt33r · 2017-09-06T11:58:13Z

Please fill in this template.

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Follow the advice from the readme.
Avoid common mistakes.
Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If adding a new definition:

The package does not provide its own types, and you can not add them.
If this is for an NPM package, match the name. If not, do not conflict with the name of an NPM package.
Create it with dts-gen --dt, not by basing it on an existing project.
tslint.json should be present, and tsconfig.json should have noImplicitAny, noImplicitThis, and strictNullChecks set to true.

dt-bot · 2017-09-06T11:58:15Z

types/vfile/index.d.ts

Checklist

is correct naming convention?
- https://www.npmjs.com/package/vfile - https://github.com/vfile/vfile#readme
has a test file? (vfile-tests.ts or vfile-tests.tsx)
pass the Travis CI test?

Rokt33r · 2017-09-06T11:59:23Z

@wooorm (Author of VFile) Please check this.

wooorm · 2017-09-06T12:03:11Z

types/vfile/index.d.ts

+declare namespace vfile {
+    type Contents = string | Buffer;
+
+    interface Position {


Use Point as a name for this interface.

wooorm · 2017-09-06T12:03:23Z

types/vfile/index.d.ts

+        column: number;
+    }
+
+    interface Location {


Use Position as a name for this interface.

wooorm · 2017-09-06T12:04:30Z

types/vfile/index.d.ts

+    }
+
+    interface VFileError {
+        ruleId: string;


Can be null.

wooorm · 2017-09-06T12:04:40Z

types/vfile/index.d.ts

+        line: number;
+        column: number;
+        location: Location;
+        source: string;


Can be null.

wooorm · 2017-09-06T12:05:23Z

types/vfile/index.d.ts

+        name: string;
+        file: string;
+        reason: string;
+        line: number;


Can be null.

wooorm · 2017-09-06T12:05:29Z

types/vfile/index.d.ts

+        file: string;
+        reason: string;
+        line: number;
+        column: number;


Can be null.

wooorm · 2017-09-06T12:05:58Z

types/vfile/index.d.ts

+        column: number;
+        location: Location;
+        source: string;
+        fatal: boolean;


Can be null or undefined for informational messages.

wooorm · 2017-09-06T12:06:50Z

types/vfile/index.d.ts

+        history: string[];
+        data: {};
+        messages: VFileError[];
+        contents: string;


Shouldn’t this be Contents?

wooorm · 2017-09-06T12:07:57Z

types/vfile/index.d.ts

+        dirname: string;
+        basename: string;
+        stem: string;
+        extname: string;


path, basename, stem, extname, dirname can all be undefined if no path is set yet.

It can take both Position and Point

Rokt33r · 2017-09-06T13:05:45Z

types/vfile/index.d.ts

+
+    type Fail = (reason?: string, position?: Point | Position, ruleId?: string) => void;
+
+    type Info = (reason?: string, position?: Point | Position, ruleId?: string) => void;


@wooorm I've also found, position can be Point or Position. Do I understand right?

Yes! Or a Node!

It also can be Node. So, I've extracted type for this argument as StringifiablePosition, which is `Point | Position | Node`.

Original properties must be kept!

Rokt33r · 2017-09-06T16:03:29Z

@wooorm

I found vfile instance and its property, data, are extensible. So, I've added 2 generics to customize their typings. Now, type assertions against the below example are working strictly.

import vfile = require("vfile");

const file = vfile({
    custom: 'Custom tango',
    data: {
        custom: 12345
    }
});

// Typings of the custom data can be resolved from the parameters above
const custom: string = file.custom; // 'Custom tango'
const dataCustom: number = file.data.custom; // 12345

// Typings of original properties must be kept
const fileWithWrongParams = vfile({ path: 1234 }); // $ExpectError

Could you check it again? This should be the last review request.

wooorm

Looks good! I don’t use typings, so I’m not entirely sure though!

wooorm · 2017-09-06T16:22:14Z

types/vfile/index.d.ts

+    interface Node {
+        type: string;
+        data?: {};
+        position?: Position;


Should this have things like children, value, or basically anything else too?

Yeah, it should be better to have them.

children?: Node[]; value?: string;

It should be fine, right?

I think so! Nodes are allowed to have any property though. HAST includes some other props actually in use.

I see. Let's just keep only children and value for now.

To implement typings for the extra properties of HAST, we can extends from the interface

vfile doesn’t care about any other properties. Just that it’s an object with a position, and it’ll use that:

https://github.com/vfile/vfile/blob/86d0c4a17460f3788be7994b182ae8a3bc4207b5/index.js#L169-L171

So you could also allow any other properties!

I see. The message method doesn't seem to care other values except the same properties of Position.
So, I think we should delete Node interface and exploit generics again.

<P extends Position>(reason: string, position?: Point | P, ruleId?: string) => VFileError;

Now the type checking works as our intention. :)

with invalid start

with Extra value

wooorm · 2017-09-06T16:23:10Z

types/vfile/index.d.ts

+
+    type Message = (reason: string, position?: StringifiablePosition, ruleId?: string) => VFileError;
+
+    type Fail = (reason?: string, position?: StringifiablePosition, ruleId?: string) => void;


fail throws the error btw!

As long as I know, we cannot define function types that always throw an error.

But, it should be better to use never for return type rather than void.
https://basarat.gitbooks.io/typescript/docs/types/never.html

Yes!

A function always throws (e.g. in function `foo(){throw new Error('Not Implemented')}` the return type of `foo` is never)

https://basarat.gitbooks.io/typescript/docs/types/never.html

Delete `Node` interface because we are needed to check the properties about position only.

wooorm · 2017-09-06T17:24:57Z

👍👍👍

Position is not supposed to be extensible, but Node is. Testing cases for this are also added.

Rokt33r · 2017-09-06T18:05:04Z

@wooorm Sorry, I misunderstood! In the previous implementation, I made Position extensible, not Node.

So, I bring back Node and add definition test of it. It should be fine now.

3dd0a90#diff-e479b3678f226bc2d6568e43964a5c74

@sandersn
I mistakenly created another PR for VFile. And the previous one already has been accepted without reviewing any others. Could you give me some instruction how should I deal with it?

One of case must use point

sandersn · 2017-09-06T18:08:29Z

Sorry, I didn't see this PR because @typescript-bot hasn't labelled it yet. I think the best thing is to merge from master and then undo the changes you don't want to keep from the previous PR.

Rokt33r · 2017-09-06T18:19:37Z

@sandersn Okay, I'll. Thanks for the instruction. 💯

@wooorm There seems another PR for this library, and already merged into master.
Our one should be better because you reviewed, but the previous one also has lots of things good to have.(Detailed comment, Usage of Unist typings) 👯

@bizen241 wooorm and I are also working typings for this. I want you to review this PR after I fix the conflict. I'll ping you later when ready. 🥇

Should test with Node type with extra properties.

Rokt33r · 2017-09-06T18:50:32Z

@bizen241, @wooorm I've finished. 💃
Let me know if there are still something to be fixed. I'll deal with it after I wake up.

wooorm · 2017-09-06T19:19:33Z

types/vfile/vfile-tests.ts

+    }
+};
+// Accept Point
+file.message('test', position);


Shouldn’t this one be startPoint?

Yea, you're right. I've already fixed it since f161e0e.

Sorry for confusing you.

bizen241 · 2017-09-06T20:30:43Z

@wooorm @Rokt33r I'm very sorry for my mistakes.
I should not have added typings without check by author.

I think my typing definition had a lots of problems.
But your typings is very good.

I added a definition of Unist in #19472 a few days ago.
I'm sorry but please check it.

Thank you for waiting and please accept my apologies.

for review: 👍

`data` also can be decided by `C`.

Rokt33r · 2017-09-07T01:13:31Z

@bizen241 I'm okay. I don't think it is your fault. 😄
@sandersn I think it is ready to be shipped. Could you check this?

…ept contents

sandersn · 2017-09-07T15:38:37Z

Well, I guess CI is back. It found some lint errors: an unused generic N somewhere in the code. After that I think it will be ready to go.

Rokt33r · 2017-09-07T15:46:39Z

Fixed!

sandersn · 2017-09-07T15:47:02Z

Thanks @Rokt33r!

kachkaev · 2018-02-26T17:25:38Z

Hey @Rokt33r! Would you be keen to update definitions for VFile 2.3?

See https://github.com/vfile/vfile/releases/tag/2.3.0

Rokt33r · 2018-02-27T00:18:10Z

@kachkaev Yes, I would. 👍

Add VFile

122500f

wooorm suggested changes Sep 6, 2017

View reviewed changes

Rokt33r added 2 commits September 6, 2017 21:54

Fix typings for vfile by the guide of the author

7eb5ba8

Add more type of argument for position

08c0fcf

It can take both Position and Point

Rokt33r commented Sep 6, 2017

View reviewed changes

Rokt33r added 4 commits September 6, 2017 23:13

Add more option for position arguments

61cf931

It also can be Node. So, I've extracted type for this argument as StringifiablePosition, which is `Point | Position | Node`.

Exploit generics to assert typings of custom data

34b2133

Add failure scenario

ec9ef2c

Original properties must be kept!

Update comment

65698d1

wooorm reviewed Sep 6, 2017

View reviewed changes

Rokt33r added 2 commits September 7, 2017 02:04

Use never because it always throw an error

0f323ca

https://basarat.gitbooks.io/typescript/docs/types/never.html

Accept Position with extra properties

798233d

Delete `Node` interface because we are needed to check the properties about position only.

Fix rollback Node

3dd0a90

Position is not supposed to be extensible, but Node is. Testing cases for this are also added.

Fix wrong variable name

f161e0e

One of case must use point

Rokt33r added 2 commits September 7, 2017 03:44

Fix conflicts

fc6a0fb

Fix a test case by the description

9a6f7da

Should test with Node type with extra properties.

Rokt33r changed the title ~~Add VFile~~ Update VFile Sep 6, 2017

wooorm reviewed Sep 6, 2017

View reviewed changes

Delete generic D which was represented as data property.

37207e1

`data` also can be decided by `C`.

Pass generic directly if it instantiate without any other options exc…

e59a1c2

…ept contents

Remove unused generic N

34bc9e6

sandersn merged commit 19d0354 into DefinitelyTyped:master Sep 7, 2017

Rokt33r deleted the vfile branch September 8, 2017 00:30


		type Fail = (reason?: string, position?: Point \| Position, ruleId?: string) => void;

		type Info = (reason?: string, position?: Point \| Position, ruleId?: string) => void;


		type Message = (reason: string, position?: StringifiablePosition, ruleId?: string) => VFileError;

		type Fail = (reason?: string, position?: StringifiablePosition, ruleId?: string) => void;

Update VFile #19583

Update VFile #19583

Conversation

Rokt33r commented Sep 6, 2017

dt-bot commented Sep 6, 2017

Rokt33r commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rokt33r commented Sep 6, 2017 • edited

wooorm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rokt33r Sep 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rokt33r Sep 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wooorm commented Sep 6, 2017

Rokt33r commented Sep 6, 2017 • edited

sandersn commented Sep 6, 2017

Rokt33r commented Sep 6, 2017 • edited

Rokt33r commented Sep 6, 2017 • edited

Choose a reason for hiding this comment

Rokt33r Sep 6, 2017 • edited

Choose a reason for hiding this comment

bizen241 commented Sep 6, 2017

Rokt33r commented Sep 7, 2017

sandersn commented Sep 7, 2017

Rokt33r commented Sep 7, 2017

sandersn commented Sep 7, 2017

kachkaev commented Feb 26, 2018

Rokt33r commented Feb 27, 2018

Rokt33r commented Sep 6, 2017 •

edited

Rokt33r Sep 6, 2017 •

edited

Rokt33r Sep 6, 2017 •

edited

Rokt33r commented Sep 6, 2017 •

edited

Rokt33r commented Sep 6, 2017 •

edited

Rokt33r commented Sep 6, 2017 •

edited

Rokt33r Sep 6, 2017 •

edited