Skip to content

Import types from mdast or add @types/unist to devDeps #14

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

Closed
CanRau opened this issue Jan 28, 2020 · 19 comments
Closed

Import types from mdast or add @types/unist to devDeps #14

CanRau opened this issue Jan 28, 2020 · 19 comments
Labels
🙅 no/wontfix This is not (enough of) an issue for this project

Comments

@CanRau
Copy link

CanRau commented Jan 28, 2020

I think those types should probably be imported from 'mdast'
https://github.com/syntax-tree/unist-util-is/blob/master/index.d.ts#L3
Or as mentioned in the title @types/unist needs to be added to the dependencies

I'm trying to update to Yarn2 and this popped up

../../.yarn/cache/unist-util-is-npm-4.0.1-2754fc9f60-1.zip/node_modules/unist-util-is/index.d.ts(3,28): error TS2307: Cannot find module 'unist'.

and as Yarn2 is really specific about deps my only guess right now is this issue 😁

(edited by @wooorm to refactor formatting)

@CanRau CanRau added 🐛 type/bug This is a problem 🙉 open/needs-info This needs some more info labels Jan 28, 2020
@ChristianMurphy
Copy link
Member

ChristianMurphy commented Jan 28, 2020

I think those types should probably be imported from 'mdast'

I'm confused how mdast comes into the picture here.
The warning message you shared is related to unist.

Or as mentioned in the title @types/unist needs to be added to the dependencies

This is similar to syntax-tree/unist-util-filter#4

We've avoided adding direct dependencies on @types/ packages due to the issues outlined in unifiedjs/unified#45 .
When using Typescript add the unist typings using npm install --save-dev @types/unist or yarn add --dev @types/unist.

@CanRau
Copy link
Author

CanRau commented Jan 28, 2020

Hum I see, my package has @types/unist already in devDeps. Yarn2 really expects every package specifying deps specifically itself tho.
Yea my fault mdast doesn't make sense here, not sure about my thinking back then 😅 I mis-looked and thought it'd export the needed types too for some reason..
So not sure how to proceed with yarn2 🤔 resolutions don't seem to work here

@ChristianMurphy
Copy link
Member

ChristianMurphy commented Jan 28, 2020

Yarn 2 checks the typings during the install phase? Is that expected?
I haven't come across documentation briefly scanning the new docs at https://next.yarnpkg.com, do you happen to know if there is documentation for the new types/validation behavior?

Also note that Yarn 2 is still a pre-release there may be some bugs/instability.

@ChristianMurphy
Copy link
Member

/cc @Rokt33r

@ChristianMurphy ChristianMurphy added ☂️ area/types This affects typings 🐛 type/bug This is a problem 📦 area/deps This affects dependencies 🙉 open/needs-info This needs some more info and removed 🐛 type/bug This is a problem 🙉 open/needs-info This needs some more info labels Jan 28, 2020
@CanRau
Copy link
Author

CanRau commented Jan 28, 2020

You're right it doesn't, that happened because of a postinstall script in the workspaces root.
Disabling it let me proceed with yarn install though trying to build those packages directly or using lerna later yields the same result.
As mentioned I've already got unist in local devDeps, moving to dependencies doesn't change.

Yea I know about pre-release, yet they mention the explicit dependency resolution everywhere so I doubt that'll change 🤷‍♂️

Forgot to link to https://yarnpkg.com/advanced/migration#a-package-is-trying-to-access-another-package-
Update
Okay so the in the link mentioned packageExtensions are at least a fix

packageExtensions:
  'unist-util-is@*':
    dependencies:
      '@types/unist': '*'

That seems to work

@ChristianMurphy
Copy link
Member

Interesting, based off how that works.
It sounds like every project using Yarn 2, would need to:
Apply this patch for every package that relies on the typings for @types/unist and @types/vfile.

Which could be a lot when we get to the high level packages like remark/rehype/retext presets, which include many plugins, which include many utils, each of which will need to be patched.

It feels like a bad developer experience because it both takes patching, and the patching would need to happen at the nested/transitive dependency level, meaning devs need to dive deep into internals to get simple presets to work.

@ChristianMurphy
Copy link
Member

Maybe we could re-add the dependencies, and document something like using https://github.com/gcanti/unknown-ts for systems that need to use unsupported Typescript versions?

@ChristianMurphy
Copy link
Member

/cc @damonmaria @ybiquitous

@damonmaria
Copy link

The issue I had was due to the (non-dev) dependency on @types/node causing a Typescript conflict in the definition of console and FormData with other @types packages I had. I'm assuming that problem will continue.

@ybiquitous
Copy link

I agree with @damonmaria. For non-TypeScript users, I think generally it better not to add extra dependencies.

I assume that the need of the addition of some dependencies such as unknown-ts also would make worse some developers' experience.

@ChristianMurphy
Copy link
Member

Hmmm, that makes dependencies a bad option. 🤔
The devDependencies are also causing an issue.
@Rokt33r thoughts on making the typings peerDependencies? 💭

@Rokt33r
Copy link
Contributor

Rokt33r commented Feb 3, 2020

@ChristianMurphy
I absolutely prefer to use both peerDependencies and devDependencies. It makes sense more in my opinion although typescript adopters need to install @types/unist manually.

@wooorm
Copy link
Member

wooorm commented Feb 3, 2020

I’m up for going with whatever is common sense in TS-world, but I personally don’t like the caveats of https://docs.npmjs.com/files/package.json#peerdependencies.

  • peerDependencies are sometimes installed for non-TS users
  • peerDependencies are not always installed for TS users
  • peerDependencies will cause errors for non-TS users if they don’t install the peers themselves
  • Say both unist-util-x and unist-util-y have a peerDependencies of unist-util-z, but with different version ranges, how do you handle that? What should the user install? Can npm/yarn handle this?

@ChristianMurphy
Copy link
Member

I absolutely prefer to use both peerDependencies and devDependencies. It makes sense more in my opinion although typescript adopters need to install @types/unist manually.

I understand this perspective, depDependencies is going to be problematic with Yarn 2.
https://dev.to/arcanis/introducing-yarn-2-4eh1 has been making the rounds and creating a lot of interest.
The issue here is strict package boundaries https://dev.to/arcanis/introducing-yarn-2-4eh1#strict-package-boundaries

Packages aren't allowed to require other packages unless they actually list them in their dependencies. [...] Nowadays, very few packages still have compatibility issues with this rule.

Because we are using devDependencies, unist-util-is (and likely many more unified packages) have this issue.
With Yarn 2 and TypeScript 3 both getting hyped and gaining adoption this is likely the first of many issues asking the same question.
Anecdotally, two teams at my company using TypeScript and Unified + Remark/MDX, mentioned interest in upgrading to Yarn 2.

It would be good to get ahead of this.
Ideally by finding a way to ensure the types are considered inside the strict package boundary by default.
Less ideally, by creating a guide on the unified website for how to work around the issue we can link issues to as they come up.

I’m up for going with whatever is common sense in TS-world

Some packages do include @types in dependencies some don't.
Storybook for example does https://github.com/storybookjs/storybook/blob/212e1fb41fc45a20290f95f867fa382f7e6fe206/addons/links/package.json#L36

I personally don’t like the caveats of [peerDependencies]

These differences in handling peer dependencies are mostly between NPM versions, or differences between Yarn and NPM?

@wooorm
Copy link
Member

wooorm commented Feb 3, 2020

Some packages do include @types in dependencies some don't.

Seems like everyone will have this problem with Yarn 2 in the TS world. In these cases I would typically wait a bit to see what others are doing. (Maybe that’s already being figured out!)


These differences in handling peer dependencies are mostly between NPM versions, or differences between Yarn and NPM?

Pt. 1 and 2 are based on npm docs. I believe npm also does pt. 3 (but may warn about it). Pt. 3 and 4 are based on yarn docs, so may be different.
It may be different, because I don’t use peer deps currently. I believe remark-react had react as a peer and that was problematic ages ago.


By the way, as an aside, it’s interesting how engines and peers come up a lot recently, I see similarities between them

@wooorm
Copy link
Member

wooorm commented May 16, 2020

@ChristianMurphy Any news on what the teams at your company / the world decided to do? How did it go?

@ChristianMurphy
Copy link
Member

Any news on what the teams at your company [...] decided to do?

Most teams are on or have migrated back to npm, a couple are still on yarn v1 and explored v2, but ran into issues and opted to revisit later.
With npm v7 incoming with workspaces and speed improvements, which were what drew teams to yarn initially, a migration to npm v7 is looking more likely than a yarn v2 migration.

Any news on what [...] the world decided to do?

Checking FB open source, probably the largest user of yarn.
Most appear to be on yarn v1 (1, 2, 3, 4, 5).

@wooorm
Copy link
Member

wooorm commented May 16, 2020

That sounds bleak for yarn!

@wooorm
Copy link
Member

wooorm commented Feb 15, 2021

Closing because this thread does not seem to warrant changes to this project.
And it definitely sounds like an interesting challenge that anyone has, with these tools, not just us.

@wooorm wooorm closed this as completed Feb 15, 2021
@wooorm wooorm added 🙅 no/wontfix This is not (enough of) an issue for this project and removed ☂️ area/types This affects typings 🐛 type/bug This is a problem 📦 area/deps This affects dependencies 🙉 open/needs-info This needs some more info labels Feb 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙅 no/wontfix This is not (enough of) an issue for this project
Development

No branches or pull requests

6 participants