search

remarkjs / react-remark

React component and hook to use remark to render markdown
https://remarkjs.github.io/react-remark
MIT License
204 stars 7 forks source link

Fix `useRemarkSync` param type #74

Open karlhorky opened 7 months ago

karlhorky commented 7 months ago

Initial checklist

Description of changes

It looks like https://github.com/remarkjs/react-remark/pull/18 used the UseRemarkOptions type instead of the UseRemarkSyncOptions type for the 2nd parameter of the useRemarkSync() function:

It seems incorrect to me because of A) the name and B) the onError property not being used.

karlhorky commented 7 months ago

The CI failure looks unrelated?

npm WARN ERESOLVE overriding peer dependency
npm WARN While resolving: @reach/router@1.3.4
npm WARN Found: react@17.0.2
npm WARN node_modules/react
npm WARN   dev react@"^17.0.0" from the root project
npm WARN   50 more (@emotion/core, @emotion/styled, @emotion/styled-base, ...)
npm WARN 
npm WARN Could not resolve dependency:
npm WARN peer react@"15.x || 16.x || 16.4.0-alpha.0911da3" from @reach/router@1.3.4
npm WARN node_modules/@storybook/api/node_modules/@reach/router
npm WARN   @reach/router@"^1.3.4" from @storybook/api@6.3.4
npm WARN   node_modules/@storybook/api
npm WARN 
npm WARN Conflicting peer dependency: react@16.14.0
npm WARN node_modules/react
npm WARN   peer react@"15.x || 16.x || 16.4.0-alpha.0911da3" from @reach/router@1.3.4
npm WARN   node_modules/@storybook/api/node_modules/@reach/router
npm WARN     @reach/router@"^1.3.4" from @storybook/api@6.3.4
npm WARN     node_modules/@storybook/api
npm WARN ERESOLVE overriding peer dependency
npm WARN While resolving: @reach/router@1.3.4
npm WARN Found: react-dom@17.0.2
npm WARN node_modules/react-dom
npm WARN   dev react-dom@"^17.0.0" from the root project
npm WARN   31 more (@storybook/addon-actions, ...)
npm WARN 
npm WARN Could not resolve dependency:
npm WARN peer react-dom@"15.x || 16.x || 16.4.0-alpha.0911da3" from @reach/router@1.3.4
npm WARN node_modules/@storybook/api/node_modules/@reach/router
npm WARN   @reach/router@"^1.3.4" from @storybook/api@6.3.4
npm WARN   node_modules/@storybook/api
npm WARN 
npm WARN Conflicting peer dependency: react-dom@16.14.0
npm WARN node_modules/react-dom
npm WARN   peer react-dom@"15.x || 16.x || 16.4.0-alpha.0911da3" from @reach/router@1.3.4
npm WARN   node_modules/@storybook/api/node_modules/@reach/router
npm WARN     @reach/router@"^1.3.4" from @storybook/api@6.3.4
npm WARN     node_modules/@storybook/api
npm WARN ERESOLVE overriding peer dependency
npm WARN While resolving: create-react-context@0.3.0
npm WARN Found: react@17.0.2
npm WARN node_modules/react
npm WARN   dev react@"^17.0.0" from the root project
npm WARN   50 more (@emotion/core, @emotion/styled, @emotion/styled-base, ...)
npm WARN     node_modules/@storybook/router
npm WARN ERESOLVE overriding peer dependency
npm WARN While resolving: @reach/router@1.3.4
npm WARN Found: react-dom@17.0.2
npm WARN node_modules/react-dom
npm WARN   dev react-dom@"^17.0.0" from the root project
npm WARN   31 more (@storybook/addon-actions, ...)
npm WARN 
npm WARN Could not resolve dependency:
npm WARN peer react-dom@"15.x || 16.x || 16.4.0-alpha.0911da3" from @reach/router@1.3.4
npm WARN node_modules/@storybook/router/node_modules/@reach/router
npm WARN   @reach/router@"^1.3.4" from @storybook/router@6.3.4
npm WARN   node_modules/@storybook/router
npm WARN 
npm WARN Conflicting peer dependency: react-dom@16.14.0
npm WARN node_modules/react-dom
npm WARN   peer react-dom@"15.x || 16.x || 16.4.0-alpha.0911da3" from @reach/router@1.3.4
npm WARN   node_modules/@storybook/router/node_modules/@reach/router
npm WARN     @reach/router@"^1.3.4" from @storybook/router@6.3.4
npm WARN     node_modules/@storybook/router
npm WARN ERESOLVE overriding peer dependency
npm WARN While resolving: create-react-context@0.3.0
npm WARN Found: react@17.0.2
npm WARN node_modules/react
npm WARN   dev react@"^17.0.0" from the root project
npm WARN   50 more (@emotion/core, @emotion/styled, @emotion/styled-base, ...)
npm WARN 
npm WARN Could not resolve dependency:
npm WARN peer react@"^0.14.0 || ^15.0.0 || ^16.0.0" from create-react-context@0.3.0
npm WARN node_modules/@storybook/router/node_modules/create-react-context
npm WARN   create-react-context@"0.3.0" from @reach/router@1.3.4
npm WARN   node_modules/@storybook/router/node_modules/@reach/router
npm WARN 
npm WARN Conflicting peer dependency: react@16.14.0
npm WARN node_modules/react
npm WARN   peer react@"^0.14.0 || ^15.0.0 || ^16.0.0" from create-react-context@0.3.0
npm WARN   node_modules/@storybook/router/node_modules/create-react-context
npm WARN     create-react-context@"0.3.0" from @reach/router@1.3.4
npm WARN     node_modules/@storybook/router/node_modules/@reach/router
npm ERR! code EUSAGE
npm ERR! 
npm ERR! `npm ci` can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with `npm install` before continuing.
npm ERR! 
npm ERR! Invalid: lock file's type-fest@0.8.1 does not satisfy type-fest@0.13.1
npm ERR! Missing: type-fest@0.8.1 from lock file
npm ERR! Missing: type-fest@0.8.1 from lock file
npm ERR! Missing: type-fest@0.8.1 from lock file
ChristianMurphy commented 7 months ago

Welcome @karlhorky 👋 I appreciate your enthusiasm and the PR, but also chill. Multiple passive aggressive @ mentions is a bit much. It's a type, documentation, I welcome improvements, but is not breaking or a blocker for your usage of the library. It is also the weekend, take it down a notch.

ChristianMurphy commented 7 months ago

To your CI/CD comment, I think npm has been tested with the legacy version of peer dependencies. The error look like npm's new handling of peer dependencies. This could either by resolved by adding the legacy peer flag in CI, or focusing on getting #39 wrapped up, which upgrades all the dependencies to newer and more compatible versions.

karlhorky commented 7 months ago

but also chill.

Multiple passive aggressive @ mentions is a bit much

I'm sorry. Was definitely not my intention to cause any stress or be aggressive and I'd like to understand more to improve. I'll try reaching out privately on Twitter/X/email.

karlhorky commented 7 months ago

This could either by resolved by adding the legacy peer flag in CI

Added a step in https://github.com/remarkjs/react-remark/pull/74/commits/0607f1034824825d5f7752afd5b9c1bb98f1c26e

(Made it a separate step to make it easier to delete later, with less noise in Git blame)

JounQin commented 7 months ago

Glad to see see again @karlhorky!

The changes look good to me.

karlhorky commented 7 months ago

Glad to help, thanks for the reviews! If there's anything else I should do to get this merged, let me know :)