-
-
Notifications
You must be signed in to change notification settings - Fork 61
Add npm
package using napi-rs
#182
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
base: main
Are you sure you want to change the base?
Conversation
Wow, cool work! It’s a lot, will take some time to review. And, also I am sure you can find stuff if you self-review. |
Here is the benchmark result on the Zod v3 mdx documentation clk: ~3.25 GHz
cpu: Apple M2 Pro
runtime: node 24.2.0 (arm64-darwin)
benchmark avg (min … max) p75 / p99 (min … top 1%)
--------------------------------------------------- -------------------------------
markdown-rs parse 16.33 ms/iter 16.57 ms █▄
(15.29 ms … 17.08 ms) 17.04 ms ▅ ▅ ▅██ ▅ ▅
( 1.21 mb … 1.22 mb) 1.21 mb ▅▁▁▁▅█▅█▅█▁█████▅▅█▁█
remark parse 42.02 ms/iter 43.75 ms ██
(39.96 ms … 46.92 ms) 46.26 ms ██
( 10.46 mb … 44.35 mb) 15.31 mb ███▁██▁▁▁▁▁▁█▁▁▁▁█▁▁█
summary
markdown-rs parse
2.57x faster than remark parse
--------------------------------------------------- -------------------------------
markdown-rs parse without parsing js 16.14 ms/iter 16.31 ms █
(15.63 ms … 16.81 ms) 16.64 ms ▅ ▅▅█ ▅ ▅ ▅
( 1.21 mb … 1.21 mb) 1.21 mb ▅▅█▅▁█▅████████▁█▅▁▅▅
remark parse 41.73 ms/iter 41.63 ms █
(39.68 ms … 47.57 ms) 45.96 ms █ ██
(841.99 kb … 14.55 mb) 11.21 mb █▁█████▁▁▁▁█▁▁▁▁▁▁▁▁█
summary
markdown-rs parse without parsing js
2.59x faster than remark parse
--------------------------------------------------- -------------------------------
markdown-rs html 7.86 ms/iter 8.03 ms ▃ █▃
(7.34 ms … 8.29 ms) 8.29 ms ▃█▆▃█▃███ ▃▃ ▃ ▃▃
(186.48 kb … 186.64 kb) 186.51 kb ▄▆▄▁█████████████████
remark html 41.38 ms/iter 42.53 ms █
(39.16 ms … 46.77 ms) 43.76 ms ▅▅▅▅█ ▅ ▅ ▅ ▅ ▅ ▅
( 6.65 mb … 14.85 mb) 12.03 mb █████▁█▁█▁▁█▁▁▁█▁█▁▁█
summary
markdown-rs html
5.27x faster than remark html
|
Notice that the bigger files like The important parts live in the |
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.
Thanks @remorses!
I appreciate the time put into this.
I am a bit cautious on the maintainer burden. (See #6 (comment) )
With NAPI and native bindings, now we need to test both across node versions and across a broad set of operating systems and instruction sets.
Many of which I don’t have a machine I could test a platform specifical bug on.
I would strongly lean towards WASM first, as its one binary that can be delivered to many native platforms and to the web, with minimal overhead (less than 1.5x difference from native).
|
||
This is the package that handles NAPI bindings for markdown-rs. It also supports WASM. It uses napi-rs to generate the npm packages published on npm. | ||
|
||
Tests use vitest, expect().toMatchInlineSnapshot() mostly. Most tests should be run with `pnpm vitest --run -u` to update snapshots, then read the test file again and make sure they have the expected result. |
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'd recommend npm
first over requiring one of the many alternative package managers (yarn, pnpm, jsr, etc).
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.
most popular projects use pnpm now, it's become the standard for workspaces and a must as soon as you hit one of the many bugs in npm, which is barely maintained now
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.
most popular projects use pnpm now
That is objectively false.
None of the top 10 packages by download use pnpm.
And most in the top 1000 don't either.
https://github.com/wooorm/npm-high-impact
More important, unified, which this is broadly part of does not https://github.com/unifiedjs/unified
soon as you hit one of the many bugs in npm
I hit far more pnpm bugs than npm
barely maintained now
Last release less than a week ago https://www.npmjs.com/package/npm?activeTab=versions
And they come at a regular cadence.
workflow_dispatch: | ||
push: | ||
paths: | ||
- napi/package.json # make new release on version change of napi package |
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.
Start with being able to release locally.
Before adding automation, I dont think we would want every merge to main to become a release.
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.
unfortunately to release a cross platform native binary you need to use the platform with the same architecture, GitHub actions is perfect for this. This code was taken for the most part from the oxc-resolver which is state of the art regarding this kind of publishing from Rust to npm
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.
Again see my main comment.
I don't think we should release platform specific versions yet.
WASM is universal.
@@ -0,0 +1,3 @@ | |||
# markdown-rs | |||
|
|||
CommonMark compliant markdown parser in Rust with ASTs and extensions. Packaged for npm with full WASM support. |
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 readme would need to be fairly complete, a lot could probably be lifted from https://github.com/remarkjs/remark or https://github.com/micromark/micromark
Which this is based off
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.
Sure, I think this can be done in a follow-up PR after the first version is released on npm
@@ -0,0 +1,54 @@ | |||
/* auto-generated by NAPI-RS */ | |||
/* eslint-disable */ | |||
export interface CompileOptions { |
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.
Avoid keeping a separate..d.ts
that needs to be maintained separate from the source.
Use JSDoc mode for TypeScript https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html
Which can both type check and generate a definitions file.
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.
This file is generated by NAPI, based on the Rust lib.rs source. It needs to be in the source code, not sure exactly why, all projects using NAPI do this.
// @ts-nocheck | ||
/* auto-generated by NAPI-RS */ | ||
|
||
const { createRequire } = require('node:module') |
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.
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 can try but I am not an expert on this and there could be bugs in NAPI when using the new esm option, because not many projects use it and it's not adopted much. If not strictly necessary I would avoid it.
I am not sure releasing a WASM only build of markdown-rs would be worth it becaues performance would be worse that remark js. I don't think there is need to test the markdown-rs build in other platforms, they will almost surely always work given this project doesn't have any dependencies on OS specific syscalls but only serde and an id generation package. |
How so?
I disagree. I'll defer to @wooorm as BDFL, but this is not something I'd be able to support. |
Fix #6
The npm package is currently published as @xmorse/markdown-rs
This npm package will also support WASM. Most of the CI has been ported from the oxc-resolver package.
Still a WIP, the exported functions need to be thought out better, for now I am experimenting.
Things to do before merge:
@xmorse/markdown-rs
tomarkdown-rs
(or other available name)