Skip to content

Conversation

@VastBlast
Copy link
Contributor

This PR completely rewrites and revamps the JS bindings. Summary of the changes:

  • The JS bindings have been rewritten using koffi to dramatically reduce complexity, fix nearly all the bugs with the current node module, and allow for easier maintenance (requiring no actual C code, some boilerplate required for structs).
    • Initial testing shows nearly no difference in performance from the previous implementation (feel free to run your own benchmarks).
    • Koffi is the only additional runtime dependency.
  • Rewritten in TypeScript and fully typed.
  • Exports minify function which is used to minify a given string or buffer.
    • This is the only usable export, other functions such as config and file have been removed. File handling should be done in the script not offloaded to the native library.
  • Now it works asynchronously and does not block the JS event loop.
  • Added new test suite using vitest with more robust testing.
  • Successfully builds on Windows, MacOS, and Linux (previously did not compile on Windows).
  • Rewrote the nodejs workflow to work with the new implementation.
    • Enabled arm64 and Windows prebuilds

Closes #517, #518, #520, #578, #727, #733, #878

Let me know if you have any questions about the design choices or if you notice any issues.

Note: this PR introduces breaking changes to the node module from the previous implementation.

@VastBlast VastBlast force-pushed the js-bindings-rewrite branch from 9f2d962 to 7aed404 Compare December 4, 2025 09:50
@tdewolff
Copy link
Owner

tdewolff commented Dec 4, 2025

Fantastic work! Thank you very much for the effort, this is a great step for the JS bindings.

One nitpick I found is that the minify function accepts a JS object with the config parameters, Data and Type. Since Type and Data are essential, I suggest those should be the first two parameters to the function, and the third would be an optional parameter for configuration options. If left out (null) it should be the default options. What do you think?

minify('text/html', data);

or

minify('text/html', data, {HTMLKeepDocumentTags: true});

@VastBlast
Copy link
Contributor Author

Fantastic work! Thank you very much for the effort, this is a great step for the JS bindings.

One nitpick I found is that the minify function accepts a JS object with the config parameters, Data and Type. Since Type and Data are essential, I suggest those should be the first two parameters to the function, and the third would be an optional parameter for configuration options. If left out (null) it should be the default options. What do you think?

minify('text/html', data);

or

minify('text/html', data, {HTMLKeepDocumentTags: true});

I was debating writing it like this initially. I think this would've been good if there was only 1 required argument, but since it requires both data and type, I thought it would be less readable/harder to remember the arg order vs the explicit data:.., type: 'text/html'.

As a single config option, it's easier for code that builds options dynamically (for example config-based tools which is a valid use case for minifiers).

What if we kept the current object but also included an optional overload? So the library would export both:
minify(type, data, optionalOptions?) and minify(options)

Many popular ts modules use this pattern too, so I think this would be best. If you prefer, maybe we can document the main signature as minify(type, data, optionalOptions?) while keeping both options.

@tdewolff tdewolff merged commit 8cf9eb3 into tdewolff:master Dec 4, 2025
19 checks passed
@tdewolff
Copy link
Owner

tdewolff commented Dec 4, 2025

Excellent, merged, thank you very much for the work. I haven't been able to test it but I suppose it is all right.

@VastBlast
Copy link
Contributor Author

Excellent, merged, thank you very much for the work. I haven't been able to test it but I suppose it is all right.

Awesome! If any issues pop up feel free to tag me

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants