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

Remove boundListener #3

Merged
merged 3 commits into from
Oct 2, 2024
Merged

Remove boundListener #3

merged 3 commits into from
Oct 2, 2024

Conversation

blakeembrey
Copy link
Contributor

Following up from pillarjs/path-to-regexp#333, it appears some users are passing Express.js instances directly to this library. Express.js has a method called bind (from node.js require('http').METHODS) so this is not the native method. For a very long time this has basically been a noop because it does a return this, but recently an implicit string coercion was removed for all the app[METHODS] methods, so this line now causes an error.

Opening to start a discussion since issues are disabled.

@CLAassistant
Copy link

CLAassistant commented Sep 30, 2024

CLA assistant check
All committers have signed the CLA.

wll8 added a commit to wll8/mockm that referenced this pull request Oct 1, 2024
@pimterry
Copy link
Member

pimterry commented Oct 1, 2024

Express shadows native fn.bind() on request listeners? That is surprising! I can imagine that might cause a few problems.

This logic here can't safely be removed unfortunately - as described in the comment there, we need to ensure that within the 'request' event listener, this refers to the server you're listening to (and conceptually here that's httpolyglot, not the otherwise-invisible internal server instances). Dropping that will break various listener patterns elsewhere.

Should be an easy fix though - can you update this to use Function.prototype.bind instead, with a comment to explain why?

Looks like CI is failing due to unrelated type changes elsewhere - I'll sort that shortly, don't worry about it CI is failing due to type issues from this PRs change actually, the previous types can just be restored once we're using the prototype method directly instead.

@blakeembrey
Copy link
Contributor Author

@pimterry Updated. For any express server this line has not worked to bind a function and just returned this. It's probably not too widespread in other frameworks, directly generating methods from http.METHODS feels like something that should be avoided.

@pimterry pimterry merged commit caa5c6e into httptoolkit:main Oct 2, 2024
10 checks passed
@pimterry
Copy link
Member

pimterry commented Oct 2, 2024

Thanks @blakeembrey! Merged & released as v2.2.2.

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.

3 participants