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

Replace traverse with neotraverse #443

Merged
merged 1 commit into from
Jul 17, 2024
Merged

Replace traverse with neotraverse #443

merged 1 commit into from
Jul 17, 2024

Conversation

wojtekmaj
Copy link
Contributor

This PR replaces traverse with neotraverse, a fork and TypeScript rewrite of traverse with 0 dependencies (as opposed to 66: https://npmgraph.js.org/?q=traverse) and lots of improvements.

LuchoTurtle
LuchoTurtle previously approved these changes Jul 17, 2024
Copy link
Member

@LuchoTurtle LuchoTurtle left a comment

Choose a reason for hiding this comment

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

Thank you @wojtekmaj for the PR, it is really appreciated!
Was not aware of https://github.com/puruvj/neotraverse, it seems to be a great library from the original traverse.
Since this looks like a simple change and since neotraverse is 1-1 compatible with traverse, I'm approving these changes.

Tagging @nelsonic to give a quick look over and we'll get it merged ASAP :)

@PuruVJ
Copy link
Contributor

PuruVJ commented Jul 17, 2024

Looks awesome! I just checked this lib on npmgraphs, and the entire middle part will be gone after this PR is merged and released 😄

EDIT: Well a big chunk should be gone, but not everything, as even aws-sdk relies on some of these shared micropolyfills

CleanShot 2024-07-17 at 10 52 53

Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

@wojtekmaj thanks for the PR! 🙏
@LuchoTurtle thanks for tagging. 🏷️ 👌
I'm all for PRs that reduce our dependency tree! 🎉

Never heard of neotraverse ... 🤷‍♂️
Prolly cause it was first published 6 days ago! 💭

https://www.npmjs.com/package/neotraverse?activeTab=versions 👀
image

Currently has very low usage: https://www.npmjs.com/package/neotraverse
image

By contrast traverse is "community tested": https://www.npmjs.com/package/traverse
image

If we merge this PR we will 200x the neotraverse download stats ... 📈
https://www.npmjs.com/package/aws-sdk-mock
image

Looks reasonably well-documented: https://github.com/PuruVJ/neotraverse
image
Only two contributors ... https://github.com/PuruVJ/neotraverse/graphs/contributors
image

@PuruVJ seems legit.
image

@Aslemammad also seems technically competent! 👌
image

My only "concern" is this:
New modules appearing to "fix" the problems of older/bloated ones is great!
But it's a massive attack vector for getting malicious code onto the computers of key engineers using AWS to deploy their apps!

Right now the code for neotraverse https://github.com/PuruVJ/neotraverse/tree/main/src looks fine!
But if a malicious person can inject code into it, they could steal AWS keys on dev machines, spin up $$$$$$ of machines on AWS and then erase their tracks ...

We definitely don't want to be in the news, e.g:
https://www.trendmicro.com/vinfo/mx/security/news/cybercrime-and-digital-threats/hacker-infects-node-js-package-to-steal-from-bitcoin-wallets

So while I appreciate the enthusiasm, I am naturally cautious/paranoid. 💭

@wojtekmaj how did you find out about this package?

@PuruVJ
Copy link
Contributor

PuruVJ commented Jul 17, 2024

Hi! I created neotraverse specifically because traverse itself got extremely bloated between 0.6.8 and 0.6.9, going from 1.4KB to 21KB and 0 dependencies to 66. It's an extremely concerning problem. neotraverse aims to follow the exact API for traverse forever(barring neotraverse/modern which requires rewriting your own code, so the gun is in ur hand, not mine, metaphorically speaking).

traverse has a much bigger surface area for an attack vector simply because of 66 dependencies. Granted, all of them belong to one person, but 1 dependency belonging to one person(neotraverse to myself) is still less risky than 66 all belonging to one person. All you need is one dependency at 6th level deep to go awry and everything breaks down, while if the main package(neotraverse) gets compromised, it will be with a version bump, and you can always trace the direct dependencies down rather than transitive ones.

Regarding authenticity of myself, I have multiple open source packages, namely neodrag(which is 5 libraries), neoconfetti(5 libs again), neocodemirror(1 lib at the moment), all-of-just and a few more. I am on Svelte core team as well, as can be confirmed by visiting the repo 😄

If we merge this PR we will 200x the neotraverse download stats ... 📈
https://www.npmjs.com/package/aws-sdk-mock

I am not after any vanity metrics like these, they don't matter.

If you still have reservations about me introducing attack vector, the best I can do is give you my word it won't happen. I myself have no mal-intentions, neither do I let anyone make any change to my projects without scrutinizing it to death and back 😅

Being the devil's advocate, you can stick to traverse, and it will be perfectly fine in the end. The user who owns the 66 packages will most probably not introduce an attack vector or anything like that. Only concern is that it's going to become bigger with time, as I noted here: https://puruvj.dev/blog/forking-and-fixing-traverse#Deeper-issue

Some references:
0.6.8 -> 0.6.9 bloat discovery: https://x.com/passle_/status/1810805530706792930
Launching neotraverse just 1 day later: https://x.com/puruvjdev/status/1811382220038234296
Builder.io uses it in Mitosis: https://x.com/puruvjdev/status/1813262143384658279

Let me know your thoughts

@PuruVJ
Copy link
Contributor

PuruVJ commented Jul 17, 2024

Thank you @wojtekmaj for the PR, it is really appreciated! Was not aware of https://github.com/puruvj/neotraverse, it seems to be a great library from the original traverse. Since this looks like a simple change and since neotraverse is 1-1 compatible with traverse, I'm approving these changes.

Tagging @nelsonic to give a quick look over and we'll get it merged ASAP :)

I would suggest building this PR. While the API itself is identical, neotraverse doesn't ship with CommonJS, which could break the entire build. we will have to switch neotraverse/legacy in that case

@wojtekmaj
Copy link
Contributor Author

wojtekmaj commented Jul 17, 2024

Hi! Thanks @nelsonic for a detailed write-up.

@wojtekmaj how did you find out about this package?

I found the package through author's Twitter: https://x.com/puruvjdev

But it's a massive attack vector for getting malicious code onto the computers of key engineers using AWS to deploy their apps!

While I definitely hear your concerns, I'm wondering, how did you manage to keep up with traverse updates, and perhaps more importantly, all 66 of its dependencies updates, until this point? Out of these 66 dependencies, 58 (!) have one maintainer.

That setup is infinitely easier to sneak shady stuff into.

One of the reason is that you will NEVER be able to lock a version for secondary dependencies - you can only do that for your own dependencies. With neotraverse, assuming (unrightfully so, in my opinion) zero trust in the maintainer, one can review the entire source code, and simply lock its version to 0.6.11, completely removing attack vector, thanks to neotraverse having zero dependencies. Even if @PuruVJ would decide to join the dark side later, it won't matter - you'll be on 0.6.11 :D

@PuruVJ
Copy link
Contributor

PuruVJ commented Jul 17, 2024

The test is breaking due to neotraverse being ESM-only. @wojtekmaj could you switch it to neotraverse/legacy?

@nelsonic
Copy link
Member

@wojtekmaj to answer your question:

how did you manage to keep up with traverse updates, and perhaps more importantly, all 66 of its dependencies updates, until this point?

We didn't even try to keep track of the dependencies in traverse
it's assumed to be "stable" and reliable as it's maintained/published by @ljharb. 🦸‍♂️
We rely - perhaps naively - on Snyk https://snyk.io/test/github/dwyl/aws-sdk-mock to monitor the deps tree.

However we've recently had a "false negative" from Snyk - in a private client project, not this one - so our attention has been drawn to security concerns ... 💭 The false negative from Snyk and unmanageable dependency trees in Node has actually caused our client CTO to totally re-evaluate their use of Node.js 🤯 and they've decided to migrate their systems to Go! 😬

Would definitely have preferred to see an attempt to improve traverse before creating a brand new module.
i.e. open an issue proposing to remove the dependencies from traverse before forking/re-writing it ...
Totally understand that sometimes it's faster to rewrite something than to deal with politics ... ⏳
But it also leads to fragmentation in the ecosystem and JavaScript fatigue ... 🙃

Anyway ... 💭

Let's get this PR updated to use neotraverse/legacy. 🙏

@PuruVJ
Copy link
Contributor

PuruVJ commented Jul 17, 2024

Would definitely have preferred to see an attempt to improve traverse before creating a brand new module.
i.e. open an issue proposing to remove the dependencies from traverse before forking/re-writing it ...
Totally understand that sometimes it's dshaw/env#8 to rewrite something than to deal with politics ... ⏳
But it also leads to fragmentation in the ecosystem and JavaScript fatigue ... 🙃

That's a giant discussion we should not have here 😅 Just know that traverse was all good before it was "sabotaged" and based on the discussion, it seems unlikely its gonna get any lighter

Anyways, thanks a lot for agreeing! Let's get this done! 🚀

This PR replaces traverse with neotraverse, a fork and TypeScript rewrite of traverse with 0 dependencies (as opposed to 66: https://npmgraph.js.org/?q=traverse) and lots of improvements.
@wojtekmaj
Copy link
Contributor Author

Sorry folks for the delay! I literally became a father since my last comment 🤣 Fixed the import to /legacy.

@LuchoTurtle
Copy link
Member

Approving the workflow. All tests pass ❤️

@wojtekmaj oh wow! Huge congratulations! You shouldn't be worried about this PR now, go enjoy the perks of fatherhood! :D

@PuruVJ
Copy link
Contributor

PuruVJ commented Jul 17, 2024

Whoaaa Huge congrats @wojtekmaj!! 🔥🎉🎉🎉

@wojtekmaj
Copy link
Contributor Author

wojtekmaj commented Jul 17, 2024

@wojtekmaj oh wow! Huge congratulations! You shouldn't be worried about this PR now, go enjoy the perks of fatherhood! :D

No worries, he's asleep 🤣 GOTTA HUSTLE

@LuchoTurtle
Copy link
Member

LuchoTurtle commented Jul 17, 2024

Now THAT is a cute one! 👶 We gotta work on creating him a GitHub account ASAP, fella oughta learn the Git ways young! 😛 (literally 5 hours after being born, that has to be some sort of record on this site xD)

@PuruVJ
Copy link
Contributor

PuruVJ commented Jul 17, 2024

Imagine Github signup doesn't allow today's date 🥲 Someone make a petition

@nelsonic
Copy link
Member

Register your kid's account with a birthday 20 years ago and in a few years time he can update it. 😉
Both our kids have GitHub accounts and they aren't allowed near a computer yet. ⌨️ 🚫
I sometimes mention them in a comment/commit giving them easter-eggs to find in the future. 🥚 ⏳

@PuruVJ
Copy link
Contributor

PuruVJ commented Jul 17, 2024

@nelsonic i believe we're waiting for your approval 👀👀

@nelsonic
Copy link
Member

@PuruVJ thanks for drawing our attention to the bloat https://x.com/passle_/status/1810805530706792930 ... 🤦‍♂️
Lame ... 😕
image

I'm all for modularity and re-use. but c'mon ... 🤷‍♂️
this spaghetti is clearly a dependency/package download count inflation exercise 📈
to get to the top of some leaderboard. 🙄 https://x.com/passle_/status/1810939058324938851
66 child dependencies instead of zero is totally the wrong direction for maintainability!
https://www.npmjs.com/~ljharb is definitely "winning" on package quantity ...
image

Anyway ... 🙊
Thanks again for drawing our attention to it. 🔍

Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

:shipit:

Thanks again for the PR @wojtekmaj and congrats! 👶 ❤️
@PuruVJ good work on keeping neotraverse low dependency. 🥇
Forking is part of Open Source. 👌
@LuchoTurtle over to you to create the new release and publish to NPM. 📦
Given that neotraverse is a drop-in-replacement with no (test) breaking changes, it prob doesn't need a major version just a minor; e.g: 6.0.4 -> 6.1.0? but up to you. 💭

@nelsonic nelsonic merged commit 74ff046 into dwyl:main Jul 17, 2024
10 checks passed
@wojtekmaj wojtekmaj deleted the neotraverse branch July 17, 2024 15:43
@wojtekmaj
Copy link
Contributor Author

Sorry for spamming you the last time, but I just gotta say, this discussion was just THE NICEST. It belongs to an Open Source Museum once someone establishes one.

@PuruVJ
Copy link
Contributor

PuruVJ commented Jul 17, 2024

Thank you all!! Excited to see gigabytes drop across entire internet thanks to this change 😄

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.

4 participants