-
Notifications
You must be signed in to change notification settings - Fork 96
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
chore(): update deps #91
Conversation
Part of my rationale for this PR is fact that webpack use neo-async. This package is commonly used in conjunction with webpack-dev-server it would make sense to not load async and neo-async |
@chyzwar - I see your point here. There was a previous PR about neo-async, it was focused on runtime performance, whereas this is focused on reducing your node_modules footprint. I'm going to brain dump, you are not the first to ask. We can follow up in conversation style, but here are my thoughts, concerns, questions, in long form (aka, ignore prose, where writing is poor). Your thoughts and feedback, for or against my concerns and conservative reasoning is much appreciated. Also, @chyzwar and please see #92, where a version 2 of portfinder, can be discussed further. Braindump on my thoughts on neo-async, don't take any of this the wrong way, it's just my freeflow thinking atm, very quickly: Given I have seen this request more than once, can you explain the benefits of swapping out neo-async for async in portfinder, besides trying to trim node dep tree? B/c operating system scheduling and other non neo-async controllable factors determine the upper bound on running time (from port open -> release), I'm trying to understand the motivation for why not only this PR but a few other tickets have requested switching out the async library for perf reasons. Regarding the size of node_modules - I don't think the minor packages we ship are really the bloat for any upstream consumers, but if that is a concern, pnp is worth a look, albeit not perfect yet by any means. I also realize neo-async is 'newer' but are there benefits (security, something else, besides perf) that are going to change the end user perception of using portfinder in a way consumers will notice (not a micro-benchmark but real world perception?) I will say that due to the popularity of neo-async, one argument for using it is its momentum and thus it may help grow the portfinder community and contributions, allowing for creative community contributions to allow for capabilities not yet invented/implemented. One could argue this is the (only?) reason to consider it, but as of yet I have not heard that argument from anyone else (besides myself due to seeing some excitement in issues and prs around neo-async in the recent past here in portfinder itself). Your last argument, that webpack uses neo-async, is noted, but (for lack of a better analogy) - node has a long history of maintaining stability, even as new paradigms and api's come out. Example - different versions of streams, which a bit of a pain to allow interop, was much less pain than modifying the node_modules tree for every application, in order to update all deps using streams 1 to streams 2 to streams 3 to async (for of). The extreme case of using browser api's in node modules that would otherwise not support them is a legit argument, I'm just looking for the same relative strength in the argument to upgrade to neo-async. Tl;dr - If you make a strong argument, I will seriously consider it. But I need justification for the decision in terms of actual consumer value. I'm trying to see your point of view, but give me more to run on. Very last comment is that anything that is not an a bug (which after a couple years, is unlikely) - will need to be released with a new semver major bump - this provides a safe way to provide the community with this pr and #86. Furthermore, it would allow us to automate making pr's to consuming libraries and see whether tests all pass. Which is why I created issue #92 to meta discuss future changes. We don't want to become stale, but we don't want to break. Also, another person to help out would greatly increase confidence when changing parts of this library, even dependencies. Leaving open but not approved for merge into 1.x branch, atm. |
node-portfinder is commonly used in the context of webpack-dev-server. webpack-dev-server and rest of webpack ecosystem is already using neo-sync. My rationale for this PR:
async@^1.5.2:
version "1.5.2"
resolved "https://registry.yarnpkg.com/async/-/async-1.5.2.tgz#ec6a61ae56480c0c3cb241c95618e20892f9672a"
integrity sha1-7GphrlZIDAw8skHJVhjiCJL5Zyo=
async@^2.4.1, async@^2.5.0:
version "2.6.3"
resolved "https://registry.yarnpkg.com/async/-/async-2.6.3.tgz#d72625e2344a3656e3a3ad4fa749fa83299d82ff"
integrity sha512-zflvls11DCy+dQWzTW2dzuilv8Z5X/pjfmZOWba6TNIVDm+2UDaJmXSOXlasHKfNBs8oo3M0aT50fDEWfKZjXg==
dependencies:
lodash "^4.17.14"
Since neo-async aim to be a drop-in replacement for async and both packages are fairly popular I think it is not a big deal to swith. At very least node-pathfinder should upgrade to async v2. I think this PR would not require major version as there would not change to API or version of node supported. |
Gotcha - I'll check the changelog for async 2.x, but worth noting the following: With yarn 0.x and npm > 2.x (might have been > 3.x), the async lib for portfinder will live under node_modules/portfinder/node_modules/async and the async lib for webpack lives under node_modules/webpack/node_modules/async - thus portfinder's async lib does not affect in any way webpacks -- this allows webpack to upgrade to async 3.x which has quite a changelog, while allowing other packages to each use their own version of the same library, even a forked version that may not even have the same api. The only time node_modules are deduped is when they have the same semver major version. Back in the day, this was solved by libs using peerDependencies when say a winston logger needed a newer version of winston to work, but didn't want to break other winston plugins. Over time and due to lack of , along with npm modules being used for the web, this did not scale and npm at the time commented that it was a future task - a couple of years went by, no fix in sight, thus yarn arrose out the ashes, fixing the problem with their lock file, which is a mirror image of the node dep tree you will see in your file system. Thus the lock file you show above is actually saying that webpack and portfinder do not use the same version of async, and therefore the performance of portfinders async v1 is never consumed by webpack. Nonetheless, there is a 1ms improvement in v2 of async due to setImmediate - that combined with the setImmediate used by socket cleanup in an event looped starved environment, aka, 100 ports * 2ms == 200ms + os scheduling - does mean we could improve perf by 1/5th, in a completely theoretical world. Running times however, require constants to stay constant, of which OS scheduling cannot provide any guarantee what so ever. I will consider it though, given the community keeps bringing neo-async up in tickets. I do think we should bump to async2.x first though, agreed. To be honest, the question really boils down to whether or not we'll break even 1 of the 2 million+ dependents that microsoft has so kindly added to github (top of screen). That said, npm audit does spit out a lot of potential security violations, so my argument for this change is one of security, not performance, which I think I've repeated in a few tickets. Speed will not change, for any project that uses this library, but security will. I'd like to approach this topic with that goal, b/c we're not going from O(n^2) to O(lg n) as we have no way to provide any proof for such a claim. Sorry for the rant, it's the stability that heavily concerns me as even the most innocuous change (from past experience) can lead a trail of issues and general loss of confidence that we want to avoid at all costs (think of the monetary cost of 2 million, however its measured, deps having to switch libraries b/c we we're concerned about speed over user confidence, when we can't measure speed accurately in this case anyway. sorry for the long comment, please do feel free to challenge any of my thoughts, open source is after all, a collaborative way to find the best of shared thoughts, i don't expect myself to be correct beyond a reasonable doubt, by any means. |
@@ -11,7 +11,7 @@ var fs = require('fs'), | |||
os = require('os'), | |||
net = require('net'), | |||
path = require('path'), | |||
async = require('async'), |
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.
after consideration, if we do merge - change the word async
to _async
or some other non reserved keyword from ecmascript
/globally
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.
Changed to _async. I am not sure this is really needed.
I switched back to async and upgraded to v2. |
thanks - ur right, _async not needed, just like "delete" can be a key in an obj, doy |
I updated the deps. I am trying to reduce the footprint of my node modules. It seems that other popular packages moved to use neo-async. I could not update debug to v4 because it would break on node 0.12.x.