Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
e69b734 to
0dfe733
Compare
0dfe733 to
58d70c1
Compare
blakeembrey
left a comment
There was a problem hiding this comment.
I'm not sure we need to do both ESM and CommonJS, just CommonJS should be fine for a package like this.
Also the TypeScript refactor changes exports but this didn't update the README.
I'd love to know why. It's totally understandable to skip it as it's just something I started using once I was maintaining hundreds of packages and got sick of the config bloat of every package, as well as keeping up with minor changes/improvements to things like |
This comment was marked as outdated.
This comment was marked as outdated.
You can just do this with TypeScript though, why not use that? I'd prefer to not have any additional plugins or extra steps too, I'm just not seeing the benefit to using |
This comment was marked as outdated.
This comment was marked as outdated.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
|
Adressing: #118 (comment) Hey @blakeembrey, Thanks for explaining your perspective. I really appreciate that you’re the one maintaining this long term, and I don’t want to make that harder for you. In hindsight, the original PR was too broad. I should have aligned with you before bundling the TypeScript rewrite with tooling and output changes. I’ve now narrowed it to just the TypeScript rewrite, and I also migrated it to use Any misinterpretation of your comments wasn’t intentional. I may have been operating from different assumptions about direction and with a bit of excitement around improving ESM, browser, and bundler support. Sorry about that. I would genuinely love to help maintain this library long term if you’re open to that. I’m not trying to drop a rewrite and disappear. I’m happy to share responsibility and support the agreed direction. |
|
Thank you. You brought some useful libraries to my attention, and I think it'd be worth rolling I also think the separation of |
| @@ -0,0 +1,61 @@ | |||
| // Adapted from file://./node_modules/typescript/lib/lib.dom.d.ts so we don't have to include the entire DOM lib | |||
There was a problem hiding this comment.
I think it'd be acceptable to rely on web worker, DOM, or node types to avoid this. WDYT? We test in node so I'm vaguely in favor of using one of the other two, since that would confirm it works outside of node using types (as we aren't yet testing non-node envs).
We can also keep this copied if you think it's the best approach.
There was a problem hiding this comment.
I’d keep this copied for now. The TextDecoder spec doesn’t change very often, so the maintenance risk feels low.
Relying on @types/node or the dom lib would weaken our type-safety, since it would implicitly allow Node- or browser-specific APIs into the surface area. Keeping this local avoids coupling us to a specific runtime environment at the type level.
I’ll also keep an eye on whether TypeScript eventually exposes a shared lib for common WHATWG APIs (URL, TextDecoder, TextEncoder, etc.). If that happens, we can switch to that and drop the copy.
|
My plan is to branch the existing main into a |
|
I’d be happy to help remove the regexes, as that was also my intention, for example with the hex decoding changes. |
Sure, I can hold off doing any work if you'd like to focus on it. Can you add benchmarks as you go too? The
I prefer to just keep main/master the latest work which makes my life easier, and branch for any legacy work instead. |
Thanks for holding off. I really appreciate it. As I mentioned before, I would be happy to help.
Let’s continue working in main or master once the 1.x release is out and this PR has been merged. If you have the time, could you open a tracking issue outlining your goals for v2? It would be great to align on things like the move to ESM, supported Node versions, overall goals, and any other breaking changes so we can discuss them early. |
This PR is a full TypeScript rewrite of the package as suggested by @blakeembrey in #104 (comment).
It also usestsdownto build a dual bundle: CommonJS for Node.js consumers and an ESM build for browsers and bundlers.This is another big step for #114.ToolingI tried to align the tooling as much as possible with Blake's@borderless/ts-scriptsbut went for not using it directly. So the package now uses:tsdownas build tool (tsdown is powered byrolldown- the same bundler/transformer as vite 8 uses)vitestas testing tool with native typescript and coverage supporteslintandtypescript-eslintarethetypeswrongandpublintfor package validationprettieras formatterhuskyfor git hooks combined withpretty-quickto format only staged filesI aligned the tooling with Blake's
@borderless/ts-scripts.It has fully backward compatible exports and does not change the modules behaviour.