express icon indicating copy to clipboard operation
express copied to clipboard

refactor: replace `once` package with internal implementation

Open Phillip9587 opened this issue 6 months ago • 3 comments

This PR removes the external once package and replaces it with a modern, internal utility function. The goal is to reduce unnecessary dependencies and simplify the codebase without altering runtime behavior.

  • Removes two dependencies from the dependency tree: once and wrappy
  • Updates server startup logic (e.g., app.listen) to use the new local once function

No functional changes expected — behavior remains consistent with the previous implementation.

🔎 Originally introduced in expressjs/express#3216

Phillip9587 avatar Jun 25 '25 20:06 Phillip9587

@bjohansebas I'm sorry I missed the earlier PR. I didn't get a chance to review it before it was closed. I thought we had a policy about leaving PRs open long enough to allow contributors to comment - maybe we can revisit that if needed.

I'd like to respond to a few points raised in #6555:

Removing this dep doesn't achieve anything because it is also used in other libraries that ship with express. We would need to duplicate this code into all of them.

I double-checked all 3 orgs, and as far as I can tell, express is the only package in our ecosystem depending on once. I also ran npm ls once across a few bigger company projects, and express is the only dependency bringing it in.

  • https://github.com/search?q=org%3Aexpressjs+once&type=code
  • https://github.com/search?q=org%3Apillarjs+once&type=code
  • https://github.com/search?q=org%3Ajshttp+once&type=code

Thanks for the PR! The change is valid, and the implementation looks fine. That said, I'd stick with the current use of once. It was added intentionally in https://github.com/expressjs/express/pull/3216 as a minimal, well-scoped alternative to ee-first. I prefer to keep small, purpose-built dependencies like this when they clearly express intent and don’t add meaningful overhead. Inlining this logic doesn’t materially improve clarity or reduce maintenance cost, so I'd opt to keep it as is.

Totally understand that reasoning. However, once is only used once in the entire repo, and pulling in two external dependencies (once and wrappy) for such a small utility seems unnecessary - especially when the equivalent logic can be clearly expressed inline in modern JavaScript. If this logic were shared across multiple packages or reused in multiple places, I’d agree there’s more justification. But in this case, I believe simplifying it locally is a reasonable tradeoff.

Please don’t close this PR just yet.
I'd really appreciate it if we could keep the discussion open a bit longer to gather more feedback. Let’s keep the conversation respectful and focused on what’s best for the project long-term.

Thanks!

Phillip9587 avatar Jun 25 '25 20:06 Phillip9587

my position on this is unchanged. please see these particular comments:

  • https://github.com/expressjs/express/pull/6555#issuecomment-2937718853
  • https://github.com/expressjs/express/pull/6555#issuecomment-2940459476

pulling in two external dependencies (once and wrappy) for such a small utility seems unnecessary

pulling in dependencies is not a bad thing, and eliminating dependencies is not necessarily a good thing

note also that the once library has a test suite, and this PR does not add any unit test coverage. (tests are generally required for PRs). that's not me saying to add tests now -- because I don't think we should land this change anyway. I'm only pointing out that tests are missing

in any case, as I said in the comment linked above, I think that further discussion is better suited for an actual discussion

ctcpip avatar Jun 25 '25 21:06 ctcpip

I still believe we shouldn't remove this dependency. I know my position isn't reflected in the PR, but I did share it on Slack, where I agreed with Jordan and, in general, with the other members who are opposed to removing this dependency to add it directly here.

bjohansebas avatar Jun 26 '25 01:06 bjohansebas