-
-
Notifications
You must be signed in to change notification settings - Fork 876
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 #675
Add missing dependency for types #675
Conversation
Codecov Report
@@ Coverage Diff @@
## main #675 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 5 5
Lines 731 731
=========================================
Hits 731 731 Continue to review full report at Codecov.
|
package.json
Outdated
"@types/unist": "^2.0.0", | ||
"comma-separated-tokens": "^2.0.0", | ||
"hast-util-whitespace": "^2.0.0", | ||
"mdast-util-to-hast": "^12.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"mdast-util-to-hast": "^12.0.0", |
Nice catch!
Could you solve the RemarkRehypeOptions
by instead importing it from remark-rehype
? That’s already a dependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The source is correctly importing it from remark-rehype
. I created microsoft/TypeScript#48242 since it seems like a TypeScript bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've narrowed this PR to just add @types/prop-types
for now. Hopefully the dependency on mdast-util-to-hast
can be fixed in upstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be fixed in upstream.
This sounds like a pnp problem. Not related to TS?
Or is TS “flatting” it when generated types from JS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The higher-level issue is relying on package hoisting. 99% of the time this isn't an issue because the file structure will look like:
node_modules
|-mdast-util-to-hast@^12.0.0
|-react-markdown
|-remark-rehype
If mdast-util-to-hast@^1.0.0
is added as a dependency it now looks like:
node_modules
|-mdast-util-to-hast@^1.0.0
|-react-markdown
|-remark-rehype
|-mdast-util-to-hast@^12.0.0
And react-markdown
no longer has access to the correct types.
Therefore the TypeScript bug is that it's rewriting import('remark-rehype').Options
as import('mdast-util-to-hast/lib').Options
when it shouldn't. This is a potential problem for any package manager, Yarn PnP is just making it painfully obvious that there's an issue.
It seems unlikely that someone would add a dependency on mdast-util-to-hast@^1.0.0
, that's just the easiest example to give. The more common cause of conflict is having workspaces and one app depends on react-markdown@^8.0.0
and the other depends on react-markdown@^7.0.0
and now you start having problems like this.
e85fbce
to
91ad6e9
Compare
This comment has been minimized.
This comment has been minimized.
Thanks, released! |
Initial checklist
Description of changes
Added dependencies on
@types/prop-types
andsince they are used in the built declaration files and produce type errors without them:mdast-util-to-hast