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

feat: Added a new rehype plugin to plugins.md #185

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

Conversation

ITZSHOAIB
Copy link

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

TODO

@github-actions github-actions bot added the 👋 phase/new Post is being triaged automatically label Nov 12, 2024
Copy link

Hi! It seems some of the things asked in the template are missing? Please edit your post to fill out everything.

  • Initial checklist (todo)
  • Description of changes (todo)

You won’t get any more notifications from me, but I’ll keep on updating this comment, and remove it when done!

If you need it, here’s the original template
<!--
  Please check the needed checkboxes ([ ] -> [x]). Leave the
  comments as they are, they won’t show on GitHub.
  We are excited about pull requests, but please try to limit the scope, provide
  a general description of the changes, and remember, it’s up to you to convince
  us to land it.
-->

### Initial checklist

* [ ] I read the support docs <!-- https://github.com/rehypejs/.github/blob/main/support.md -->
* [ ] I read the contributing guide <!-- https://github.com/rehypejs/.github/blob/main/contributing.md -->
* [ ] I agree to follow the code of conduct <!-- https://github.com/rehypejs/.github/blob/main/code-of-conduct.md -->
* [ ] I searched issues and couldn’t find anything (or linked relevant results below) <!-- https://github.com/search?q=user%3Arehypejs&type=Issues -->
* [ ] If applicable, I’ve added docs and tests

### Description of changes

TODO

<!--do not edit: pr-->

Thanks,
— bb

@ChristianMurphy
Copy link
Member

ChristianMurphy commented Nov 13, 2024

Thanks for sharing @ITZSHOAIB ! 🙇
Please see the check list above and check it off before the one below.

General feedback on what gets included in the plugins list:

Some criteria to include packages in this list.

  • Some documentation with at least some instructions on how to use the package.
  • A CI job to run tests.
  • A prepack script and CI job to build the types.
  • The package should be published to npm.

source: syntax-tree/hast#24 (comment)

For your plugin I'm seeing:

  • Documentation
  • CI to run lint and tests
  • Job to generate types
  • Published to NPM

Some additional pointers.

  1. Delivery readable JavaScript, not minified code to the registry https://github.com/ITZSHOAIB/rehype-code-group/blob/b2555a0abfd414c554f44a0111d9f913952fd0ae/package.json#L19

  2. Use wider version ranges where possible, to allow package managers to deduplicate (E.G. "rehype": "^13.0.0",) https://github.com/ITZSHOAIB/rehype-code-group/blob/b2555a0abfd414c554f44a0111d9f913952fd0ae/package.json#L31-L34

  3. Use Node16 resolution https://github.com/ITZSHOAIB/rehype-code-group/blob/b2555a0abfd414c554f44a0111d9f913952fd0ae/tsconfig.json#L5-L10

    Because node16 and nodenext are the only module options that reflect the complexities of Node.js’s dual module system, they are the only correct module options for all apps and libraries that are intended to run in Node.js v12 or later, whether they use ES modules or not.

    https://www.typescriptlang.org/docs/handbook/modules/reference.html#node16-nodenext

@ITZSHOAIB
Copy link
Author

ITZSHOAIB commented Nov 13, 2024

Thanks, @ChristianMurphy for a detailed reply.

  1. Is there any issue if I minify the package? I have also included the actual source code in the NPM package with dtsMaps. So using IDE, if users navigate they will land on the source code. Could you please help me understand what the issue can be with minified javascript?
  2. Wider range means? Lower versions?
  3. I will look into this.

And regarding test, I will add tests using suggested libraries. Thanks!

@ChristianMurphy
Copy link
Member

Is there any issue if I minify the package? I have also included the actual source code in the NPM package with dtsMaps. So using IDE, if users navigate they will land on the source code. Could you please help me understand what the issue can be with minified javascript?

A lot, and the same goes for including polyfills and down leveling to earlier versions of the JavaScript standard.
Some highlights:

Wider range means? Lower versions?

Version ranges are ~, ^, or * https://docs.npmjs.com/about-semantic-versioning#using-semantic-versioning-to-specify-update-types-your-package-can-accept

This does not mean lower default version, the package manager will still default to latest in the range.
It means a lower minimum required version, this gives the package manager more opportunities to de-duplicate if it finds an older version already included in a lockfile or as a dependency of another library

@ITZSHOAIB
Copy link
Author

ITZSHOAIB commented Nov 14, 2024

@ChristianMurphy Thanks for the time and effort. I have tried my best to update the package according to your suggestions. Please let me know if anything further required.

@ITZSHOAIB
Copy link
Author

@ChristianMurphy I have added some test cases as well. Thanks!

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (9bc5528) to head (e067575).
Report is 21 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #185   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            6         6           
  Lines          154       163    +9     
=========================================
+ Hits           154       163    +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👋 phase/new Post is being triaged automatically
Development

Successfully merging this pull request may close these issues.

3 participants