Skip to content
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

Test passing a literal object to processor.stringify #226

Closed
wants to merge 2 commits into from

Conversation

remcohaszing
Copy link
Member

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • 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.

The object type is correctly inferred as an mdast Root. We should make
sure this doesn’t break.
@remcohaszing remcohaszing added 🕸️ area/tests This affects tests 🤞 phase/open Post is being triaged manually labels Aug 15, 2023
@wooorm
Copy link
Member

wooorm commented Aug 15, 2023

This seems similar to

unified/index.test-d.ts

Lines 424 to 435 in 98ab675

const processorWithRehypeStringify = unified().use(rehypeStringify)
expectType<Processor<undefined, undefined, undefined, HastRoot, string>>(
processorWithRehypeStringify
)
expectType<UnistNode>(processorWithRehypeStringify.parse(''))
expectType<UnistNode>(processorWithRehypeStringify.runSync(mdastRoot))
expectType<UnistNode>(processorWithRehypeStringify.runSync(hastRoot))
expectType<string>(processorWithRehypeStringify.stringify(hastRoot))
// @ts-expect-error: not the correct node type.
processorWithRehypeStringify.stringify(mdastRoot)
expectType<VFile>(processorWithRehypeStringify.processSync(''))
?

@remcohaszing
Copy link
Member Author

remcohaszing commented Aug 15, 2023

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
Copy link
Member

wooorm commented Aug 15, 2023

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.,

test('`stringify`', async function (t) {

@wooorm
Copy link
Member

wooorm commented Aug 15, 2023

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

wooorm commented Aug 15, 2023

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

It’s tested here:

unified/index.test-d.ts

Lines 432 to 434 in 98ab675

expectType<string>(processorWithRehypeStringify.stringify(hastRoot))
// @ts-expect-error: not the correct node type.
processorWithRehypeStringify.stringify(mdastRoot)

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
Copy link
Member

wooorm commented Aug 15, 2023

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:

unified/index.test-d.ts

Lines 426 to 428 in 98ab675

expectType<Processor<undefined, undefined, undefined, HastRoot, string>>(
processorWithRehypeStringify
)

@remcohaszing
Copy link
Member Author

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

It’s tested here:

unified/index.test-d.ts

Lines 432 to 434 in 98ab675

expectType<string>(processorWithRehypeStringify.stringify(hastRoot))
// @ts-expect-error: not the correct node type.
processorWithRehypeStringify.stringify(mdastRoot)

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
Copy link
Member

wooorm commented Aug 15, 2023

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
Copy link
Member Author

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

@wooorm
Copy link
Member

wooorm commented Aug 15, 2023

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!

@wooorm wooorm closed this Aug 15, 2023
@wooorm wooorm added the 🙅 no/wontfix This is not (enough of) an issue for this project label Aug 15, 2023
@wooorm wooorm deleted the type-test-stringify-literal branch August 15, 2023 13:52
@github-actions

This comment has been minimized.

@github-actions github-actions bot added 👎 phase/no Post cannot or will not be acted on and removed 🤞 phase/open Post is being triaged manually labels Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🕸️ area/tests This affects tests 🙅 no/wontfix This is not (enough of) an issue for this project 👎 phase/no Post cannot or will not be acted on
Development

Successfully merging this pull request may close these issues.

3 participants