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

Make VFileContents generic to support processors that return objects #84

Closed
sandhose opened this issue Jan 31, 2020 · 3 comments
Closed
Labels
☂️ area/types This affects typings 🙅 no/wontfix This is not (enough of) an issue for this project

Comments

@sandhose
Copy link

Subject of the feature

Right now, compilers that return objects (DOM nodes, React nodes, etc.) can not be type checked correctly. I opened an issue on vfile side to make the VFileContent generic. See vfile/vfile#45.

Once fixed on vfile side, we should extend the Processor as well as the types related to plugins (Plugin, Pluggable, Preset, etc.) to have a VFile or VFileContent type parameter.

Related to #47

Problem

Compilers that do not return text are not type checkable.

Expected behaviour

Compilers should be able to override the VFileContent type somehow.

Alternatives

Casting the output of process[Sync]().contents, although it might not work with some strict TS compiler options/linters?

@sandhose sandhose added 🙉 open/needs-info This needs some more info 🦋 type/enhancement This is great to have labels Jan 31, 2020
@ChristianMurphy ChristianMurphy added ☂️ area/types This affects typings 🔍 status/open 🦋 type/enhancement This is great to have and removed 🙉 open/needs-info This needs some more info 🦋 type/enhancement This is great to have labels Jan 31, 2020
@ChristianMurphy
Copy link
Member

@sandhose this is something I've explored in the past.
A bit of context on the problem space:

In general Unified wants to avoid adding generics, for generics sake (for example #47 (comment)), generics should be verifiable.
In this case, meaning that the plugin/parser/stringifiers added to unified return, match up with the return type on unified itself.
And ideally, that the return type can be inferred purely based off those plugins/parsers/stringifiers.

Two things that make this tricky.

  1. there is nothing preventing multiple stringifiers with different return types from being registered, the typing should be able to handle this case
  2. Unified supports arrays of plugins and presets, these should not break the typing either

From my early tests, generic tuples might offer the possibility make this possible. https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-0.html

A few problems: microsoft/TypeScript#26113 and microsoft/TypeScript#5453 are incomplete, which can make extracting the types from lists of plugins and presets difficult. For an example see the following

unified/types/index.d.ts

Lines 253 to 261 in 952e15d

type PluginTuple<S extends any[] = [Settings?], P = Settings> = [
Plugin<S, P>,
/**
* NOTE: ideally this would be S instead of any[]
* As of TypeScript 3.5.2 generic tuples cannot be spread
* See: https://github.com/microsoft/TypeScript/issues/26113
*/
...any[]
]

unified loses settings type checking in this case.
the impact is limited to a single use, but for something like parsers and stringifiers, this would carry forward to through the rest of the use plugin chain, all the way down to process.

Another potential pit fall is presets.

unified/types/index.d.ts

Lines 227 to 236 in 952e15d

/**
* Presets provide a potentially sharable way to configure processors.
* They can contain multiple plugins and optionally settings as well.
*
* @typeParam P Processor settings
*/
interface Preset<S = Settings, P = Settings> {
plugins: PluggableList<P>
settings?: Settings
}

which rely on PluggableList

unified/types/index.d.ts

Lines 274 to 279 in 952e15d

/**
* A list of plugins and presets
*
* @typeParam P Processor settings
*/
type PluggableList<P = Settings> = Array<Pluggable<[any?], P>>

Which is another place we currently lose some interferencing.

@ChristianMurphy
Copy link
Member

The above doesn't necessarily mean that it can't be done.
Just that it is not a simple as adding a generic on process, processSync, and VFile.
And there may be some challenges.

@wooorm
Copy link
Member

wooorm commented Mar 25, 2020

I’m going to close this. It’s an issue that needs solving, but I think the proper solution is outlined here: vfile/vfile#45 (comment), and therefore is unrelated to changing vfile’s types for contents / value.

@wooorm wooorm closed this as completed Mar 25, 2020
@wooorm wooorm added 🙅 no/wontfix This is not (enough of) an issue for this project and removed 🔍 status/open 🦋 type/enhancement This is great to have labels Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
☂️ area/types This affects typings 🙅 no/wontfix This is not (enough of) an issue for this project
Development

No branches or pull requests

3 participants