Test passing a literal object to processor.stringify

[remcohaszing]

Initial checklist

• [x] I read the support docs
• [x] I read the contributing guide
• [x] I agree to follow the code of conduct
• [x] I searched issues and couldn’t find anything (or linked relevant results below)
• [x] If applicable, I’ve added docs and tests

Description of changes

The object type is correctly inferred as an mdast Root. We should make sure this doesn’t break.

[wooorm]

This seems similar to https://github.com/unifiedjs/unified/blob/98ab67534dd4e07fe4032afc1154c1ee8c81dd10/index.test-d.ts#L424-L435?

[remcohaszing]

It is similar, yet very different.

The tests you highlighted, assert that mdast Root satisfies the first argument of stringify(). The new test asserts that the first argument of stringify() is inferred as an mdast Root.

This would work too:

const remarkWithStringify = unified().use(remarkStringify)
declare const value: Parameters<typeof remarkWithStringify.stringify>[0]
expectType<MdastRoot>(value)

[wooorm]

If you want to test that, we should probably do it in more places. A const rootLoose = {type: 'root', children: []} around L32 and it being used in all cases from L327 - L460? Or is that going to result in children: never[]?

But I’m not sure it needs to be tested, here. The input of stringify is either a specific node such as MdastRoot (if a properly typed plugin is used), or a generic unist node (if no properly typed plugin is used). And this object is always assignable to both?

And, we have tons of tests in the test/ folder already: e.g., https://github.com/unifiedjs/unified/blob/98ab67534dd4e07fe4032afc1154c1ee8c81dd10/test/stringify.js#L10

[wooorm]

to test assignability of a hast node, you should probably pass a valid root, with an invalid element or so inside it, and check that there’s an error on that element, which wouldn’t happen if it’s a normal unist node?

[remcohaszing]

If you want to test that, we should probably do it in more places

I think to properly test it, all occurrences of mdastRoot and hastRoot should be replaced with literal. And we should make sure there is no accidental overlap between mdast and hast, so no empty roots.

But I’m not sure it needs to be tested, here. The input of stringify is either a specific node such as MdastRoot (if a properly typed plugin is used), or a generic unist node (if no properly typed plugin is used). And this object is always assignable to both?

This tests the type used to properly type a plugin.

It’s true that an MdastRoot is assignable to both a generic unist node and MdastRoot. That’s exactly the danger. We don’t want to accidentally accept any unist node. remark().stringify() should not accept HastRoot. It currently doesn’t, but that wasn’t tested.

And, we have tons of tests in the test/ folder already

test/stringify.js defines an object of type { type: string }, then passes it to stringify(). This also tests assignability, not type inference.

The tests in test/freeze.js do, but they only use unist Node.

[ChristianMurphy]

I think to properly test it, all occurrences of mdastRoot and hastRoot should be replaced with literal.

I agree we could and perhaps should test variations of literal. I'd be cautious removing the tests which use the mdast and hast node types directly, those are the types which most utilities and plugins will be providing, and it still seems more common to use utilities than fully hand code an AST literal.

[wooorm]

remark().stringify() should not accept HastRoot. It currently doesn’t, but that wasn’t tested.

It’s tested here: https://github.com/unifiedjs/unified/blob/98ab67534dd4e07fe4032afc1154c1ee8c81dd10/index.test-d.ts#L432-L434

It’s true that an MdastRoot is assignable to both a generic unist node and MdastRoot

It’s also the case that your root object w/o children is both assignable to a generic unist node and a MdastRoot, and a HastRoot

test/stringify.js defines an object of type { type: string }, then passes it to stringify(). This also tests assignability, not type inference.

Where does type inference come into play here? TS is going to assign in these cases?

[wooorm]

An on your earlier:

const remarkWithStringify = unified().use(remarkStringify)
declare const value: Parameters<typeof remarkWithStringify.stringify>[0]
expectType<MdastRoot>(value)

That feels very similar to these that we already have: https://github.com/unifiedjs/unified/blob/98ab67534dd4e07fe4032afc1154c1ee8c81dd10/index.test-d.ts#L426-L428

[remcohaszing]

remark().stringify() should not accept HastRoot. It currently doesn’t, but that wasn’t tested.

It’s tested here:

https://github.com/unifiedjs/unified/blob/98ab67534dd4e07fe4032afc1154c1ee8c81dd10/index.test-d.ts#L432-L434

I missed this one. This pretty much asserts the same thing and convinces me this PR isn’t needed. So feel free to close.

It’s true that an MdastRoot is assignable to both a generic unist node and MdastRoot

It’s also the case that your root object w/o children is both assignable to a generic unist node and a MdastRoot, and a HastRoot

test/stringify.js defines an object of type { type: string }, then passes it to stringify(). This also tests assignability, not type inference.

Where does type inference come into play here? TS is going to assign in these cases?

This is ok:

declare function stringify(node: import('mdast').Root): string

stringify({
  type: 'root',
  children: []
})

This is not ok:

declare function stringify(node: import('unist').Node): string

stringify({
  type: 'root',
  children: []  // Type error, the `children` property is excessive
})

Also it affects editor completion, hover information, etc.

[wooorm]

This is not ok:

declare function stringify(node: import('unist').Node): string

stringify({
  type: 'root',
  children: []  // Type error, the `children` property is excessive
})

I feel like that’s intentional, a feature not a bug, what we decided on when we removed support for arbitrary fields on Node from unist. That’s also why I added a bunch of NodeLike and PositionLike values to unist utilities. But now I’m leaning more towards: folks can use types. And some of those utilities should just accept unknown and go from their. And it’s also why we now have Nodes in mdast/hast/nlcst, for an enumeration of all valid nodes, instead of the abstract “Node” interface.

[remcohaszing]

Yes, the current behaviour a good thing. It was an answer to your question of where the type inference comes into play.

[wooorm]

I missed this one. This pretty much asserts the same thing and convinces me this PR isn’t needed. So feel free to close.

Alright, closing! Thanks for the discussion!

[github-actions[bot]]

Hi! This was closed. Team: If this was merged, please describe when this is likely to be released. Otherwise, please add one of the no/* labels.