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

Transformers take VFile instead of VFileCompatible #63

Merged
merged 2 commits into from
Jul 16, 2019
Merged

Transformers take VFile instead of VFileCompatible #63

merged 2 commits into from
Jul 16, 2019

Conversation

clavin
Copy link
Contributor

@clavin clavin commented Jul 15, 2019

Currently, the types for this package define that Transformers accept a file parameter that is VFileCompatible. The spec, on the other hand, states the type of the file parameter is a VFile.

The current definition is unergonomic because a plugin defined as:

export default () => (node: Node, file: VFile) => {
  // ...
};

...cannot be passed to use since file: VFile is incompatible with file: VFileCompatible (ironically):

import plugin from '...';

unified().use(plugin);

causes:

Argument of type '() => (tree: Node, file: VFile) => Node' is not assignable to parameter of type 'Attacher<[], Settings>'.
  Type '(tree: Node, file: VFile) => Node' is not assignable to type 'void | Transformer'.
    Type '(tree: Node, file: VFile) => Node' is not assignable to type 'Transformer'.
      Types of parameters 'file' and 'file' are incompatible.
        Type 'VFileCompatible' is not assignable to type 'VFile'.
          Type 'string' is not assignable to type 'VFile'. ts(2345)

Even more, existing plugins built on the unified spec expect a VFile to be passed without needing to coerce the passed value into a VFile, making the current definition inaccurate for these plugins.

This PR fixes this issue by adding some basic tests that make sure this pattern works with use & making the necessary 10-character removal to make them pass.

@codecov-io

This comment has been minimized.

@ChristianMurphy ChristianMurphy requested review from ChristianMurphy and a team July 15, 2019 19:47
@ChristianMurphy
Copy link
Member

/cc @Rokt33r

Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @clavin!
Looking at

transformers.run(node, vfile(file), done)

It appears Unified will always convert the vFileCompatible type to a vFile when handing off the the transforms.
This LGTM 👍

Copy link
Member

@Rokt33r Rokt33r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense to me. Thanks for the contributing!!! ❤️

@Rokt33r
Copy link
Member

Rokt33r commented Jul 16, 2019

I guess this can be deployed as a patch release because it is a kind of bug fix.

@wooorm wooorm merged commit a922a8d into unifiedjs:master Jul 16, 2019
@clavin clavin deleted the transformer-file-arg-type branch July 16, 2019 16:03
wooorm pushed a commit to remarkjs/remark that referenced this pull request Jul 20, 2019
Related to unifiedjs/unified#53.
Related to unifiedjs/unified#54.
Related to unifiedjs/unified#56.
Related to unifiedjs/unified#57.
Related to unifiedjs/unified#58.
Related to unifiedjs/unified#59.
Related to unifiedjs/unified#60.
Related to unifiedjs/unified#61.
Related to unifiedjs/unified#62.
Related to unifiedjs/unified#63.
Related to unifiedjs/unified#64.
Related to #426.

Reviewed-by: Titus Wormer <[email protected]>
Reviewed-by: Junyoung Choi <[email protected]>
Reviewed-by: Christian Murphy <[email protected]>

Co-authored-by: Junyoung Choi <[email protected]>
Co-authored-by: Christian Murphy <[email protected]>
@wooorm wooorm added ☂️ area/types This affects typings ⛵️ status/released 🐛 type/bug This is a problem 👶 semver/patch This is a backwards-compatible fix labels Aug 10, 2019
@wooorm wooorm added the 💪 phase/solved Post is done label May 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
☂️ area/types This affects typings 💪 phase/solved Post is done 👶 semver/patch This is a backwards-compatible fix 🐛 type/bug This is a problem
Development

Successfully merging this pull request may close these issues.

5 participants