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

Add missing dependency for types #1

Merged
merged 1 commit into from Mar 15, 2022

Conversation

Methuselah96
Copy link
Contributor

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

Fix these build errors:

ERROR in ../../.yarn/cache/mdast-util-gfm-npm-2.0.0-177231f942-70c35ee810.zip/node_modules/mdast-util-gfm/index.d.ts 12:44-70
TS2307: Cannot find module 'mdast-util-from-markdown' or its corresponding type declarations.
    10 |   options?: import('mdast-util-gfm-table').Options | undefined
    11 | ): ToMarkdownExtension
  > 12 | export type FromMarkdownExtension = import('mdast-util-from-markdown').Extension
       |                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^
    13 | export type ToMarkdownExtension = import('mdast-util-to-markdown').Options
    14 | export type Options = import('mdast-util-gfm-table').Options
    15 |

ERROR in ../../.yarn/cache/mdast-util-gfm-npm-2.0.0-177231f942-70c35ee810.zip/node_modules/mdast-util-gfm/index.d.ts 13:42-66
TS2307: Cannot find module 'mdast-util-to-markdown' or its corresponding type declarations.
    11 | ): ToMarkdownExtension
    12 | export type FromMarkdownExtension = import('mdast-util-from-markdown').Extension
  > 13 | export type ToMarkdownExtension = import('mdast-util-to-markdown').Options
       |                                          ^^^^^^^^^^^^^^^^^^^^^^^^
    14 | export type Options = import('mdast-util-gfm-table').Options
    15 |

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Mar 14, 2022
@wooorm
Copy link
Member

wooorm commented Mar 14, 2022

FromMarkdownExtension and ToMarkdownExtension are not supposed to be exported types.
Unfortunately, in TS through JSDoc, you can’t differentiate between local and exported typedefs.
One way to solve that is by putting stuff in a lib/ and exporting only intentional things from there: https://github.com/remarkjs/remark-rehype/blob/main/index.js.
I’m hoping that would also work in this case, and for your plug and play problems?

@Methuselah96
Copy link
Contributor Author

I don't think that will work because the gfmFromMarkdown and gfmToMarkdown function types still need to reference those types by importing them from mdast-util-from-markdown and mdast-util-to-markdown. Would it make sense to have a mdast-util-types package where the types could be shared without having to depend on the logic?

@wooorm
Copy link
Member

wooorm commented Mar 15, 2022

This sounds like a bunch of work that is meaningless and inexplicable to anyone else, but specifically meant to solve a TS issue which only occurs with pnp.

Say someone else makes an extension, how would they know about this?

Perhaps you can not use pnp? Or you can use skipLibCheck?

@Methuselah96
Copy link
Contributor Author

Methuselah96 commented Mar 15, 2022

This sounds like a bunch of work that is meaningless and inexplicable to anyone else, but specifically meant to solve a TS issue which only occurs with pnp.

This is not a TS or Yarn PnP issue. The issue is that this package imports a type from another package that it doesn't have a dependency on. That's an issue for the same reason it would be an issue to import a JS function from a package with npm without having a dependency on it, right?

@wooorm
Copy link
Member

wooorm commented Mar 15, 2022

My last comment was in response to your idea for introducing mdast-util-types, and I conflated this PR with some of your other PRs (notable react-markdown) in my head.

Reading this particular code again, my earliest assessment was incorrect (#1 (comment)). This utility does expose extensions. Still, my proposed lib/ would also help clean types and prevent FromMarkdownExtension and ToMarkdownExtension from being exported on their own.

@wooorm wooorm changed the title Move packages that types depend on to dependencies Add missing dependency for types Mar 15, 2022
@wooorm wooorm merged commit 13952eb into syntax-tree:main Mar 15, 2022
@wooorm wooorm added 📦 area/deps This affects dependencies 👶 semver/patch This is a backwards-compatible fix 💪 phase/solved Post is done labels Mar 15, 2022
@github-actions

This comment has been minimized.

@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Mar 15, 2022
@Methuselah96 Methuselah96 deleted the fix-type-dependencies branch March 15, 2022 13:41
@Methuselah96
Copy link
Contributor Author

Makes sense, sorry for the confusion. Is syntax-tree/mdast-util-gfm-table#2 in the same boat?

@wooorm
Copy link
Member

wooorm commented Mar 15, 2022

Thanks, released!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 area/deps This affects dependencies 💪 phase/solved Post is done 👶 semver/patch This is a backwards-compatible fix
Development

Successfully merging this pull request may close these issues.

None yet

2 participants