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

Fix useRemarkSync param type #74

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

karlhorky
Copy link

@karlhorky karlhorky commented Feb 18, 2024

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

It looks like #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 karlhorky mentioned this pull request Feb 18, 2024
5 tasks
@karlhorky

This comment was marked as resolved.

@ChristianMurphy
Copy link
Member

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
Copy link
Member

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
Copy link
Author

karlhorky commented Feb 18, 2024

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
Copy link
Author

karlhorky commented Feb 18, 2024

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

Added a step in 0607f10

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

@JounQin
Copy link
Member

JounQin commented Feb 19, 2024

Glad to see see again @karlhorky!

The changes look good to me.

Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @karlhorky

@karlhorky
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants