react-remark icon indicating copy to clipboard operation
react-remark copied to clipboard

Fix `useRemarkSync` param type

Open karlhorky opened this issue 1 year ago • 7 comments

Initial checklist

  • [x] I read the support docs
  • [x] I read the contributing guide
  • [x] I agree to follow the code of conduct
  • [x] I searched issues and couldn’t find anything (or linked relevant results below)
  • [x] If applicable, I’ve added docs and tests

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:

  • https://github.com/remarkjs/react-remark/pull/18/files#r1493819195

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

karlhorky avatar Feb 18 '24 18:02 karlhorky

The CI failure looks unrelated?

npm WARN ERESOLVE overriding peer dependency
npm WARN While resolving: @reach/[email protected]
npm WARN Found: [email protected]
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/[email protected]
npm WARN node_modules/@storybook/api/node_modules/@reach/router
npm WARN   @reach/router@"^1.3.4" from @storybook/[email protected]
npm WARN   node_modules/@storybook/api
npm WARN 
npm WARN Conflicting peer dependency: [email protected]
npm WARN node_modules/react
npm WARN   peer react@"15.x || 16.x || 16.4.0-alpha.0911da3" from @reach/[email protected]
npm WARN   node_modules/@storybook/api/node_modules/@reach/router
npm WARN     @reach/router@"^1.3.4" from @storybook/[email protected]
npm WARN     node_modules/@storybook/api
npm WARN ERESOLVE overriding peer dependency
npm WARN While resolving: @reach/[email protected]
npm WARN Found: [email protected]
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/[email protected]
npm WARN node_modules/@storybook/api/node_modules/@reach/router
npm WARN   @reach/router@"^1.3.4" from @storybook/[email protected]
npm WARN   node_modules/@storybook/api
npm WARN 
npm WARN Conflicting peer dependency: [email protected]
npm WARN node_modules/react-dom
npm WARN   peer react-dom@"15.x || 16.x || 16.4.0-alpha.0911da3" from @reach/[email protected]
npm WARN   node_modules/@storybook/api/node_modules/@reach/router
npm WARN     @reach/router@"^1.3.4" from @storybook/[email protected]
npm WARN     node_modules/@storybook/api
npm WARN ERESOLVE overriding peer dependency
npm WARN While resolving: [email protected]
npm WARN Found: [email protected]
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/[email protected]
npm WARN Found: [email protected]
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/[email protected]
npm WARN node_modules/@storybook/router/node_modules/@reach/router
npm WARN   @reach/router@"^1.3.4" from @storybook/[email protected]
npm WARN   node_modules/@storybook/router
npm WARN 
npm WARN Conflicting peer dependency: [email protected]
npm WARN node_modules/react-dom
npm WARN   peer react-dom@"15.x || 16.x || 16.4.0-alpha.0911da3" from @reach/[email protected]
npm WARN   node_modules/@storybook/router/node_modules/@reach/router
npm WARN     @reach/router@"^1.3.4" from @storybook/[email protected]
npm WARN     node_modules/@storybook/router
npm WARN ERESOLVE overriding peer dependency
npm WARN While resolving: [email protected]
npm WARN Found: [email protected]
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 [email protected]
npm WARN node_modules/@storybook/router/node_modules/create-react-context
npm WARN   create-react-context@"0.3.0" from @reach/[email protected]
npm WARN   node_modules/@storybook/router/node_modules/@reach/router
npm WARN 
npm WARN Conflicting peer dependency: [email protected]
npm WARN node_modules/react
npm WARN   peer react@"^0.14.0 || ^15.0.0 || ^16.0.0" from [email protected]
npm WARN   node_modules/@storybook/router/node_modules/create-react-context
npm WARN     create-react-context@"0.3.0" from @reach/[email protected]
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 [email protected] does not satisfy [email protected]
npm ERR! Missing: [email protected] from lock file
npm ERR! Missing: [email protected] from lock file
npm ERR! Missing: [email protected] from lock file

karlhorky avatar Feb 18 '24 18:02 karlhorky

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 avatar Feb 18 '24 18:02 ChristianMurphy

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.

ChristianMurphy avatar Feb 18 '24 19:02 ChristianMurphy

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 avatar Feb 18 '24 20:02 karlhorky

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)

karlhorky avatar Feb 18 '24 20:02 karlhorky

Glad to see see again @karlhorky!

The changes look good to me.

JounQin avatar Feb 19 '24 00:02 JounQin

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

karlhorky avatar Feb 19 '24 17:02 karlhorky