use exported methods rather than encapsulated ones #196
Conversation
In the `/lib/request.js` module, these methods are exported: ```javascript module.exports.Test = Test; module.exports.Request = Test; module.exports.agent = TestAgent; ``` Exporting these methods gives the benefit for those who need extra functionality to extend these methods as needed. However, the inner code does not use these exported modules, rather than it uses the private ones (ie `Test` directly) this handicapes the feature, as extending the exported methods practically does nothing. The proposed implementation can be seen on major code bases such as `node` and `mocha`; so i believe its a better practice over all. P.S. i needed to override `Test` module to add extra logging on each request sent, but overriding the Test method does nothing. agent instead of TestAgent
|
the PR is failing because of
which is non related to the PR i submitted. |
|
Hey @Bamieh, thanks for the PR! In my point of view, I would rather expose functionality through explicit interfaces than allow overriding of the exports, as I believe that it could cause some accidental issues. What sort of logging do you need to do for each request that can't be accomplished by logging when making the requests? |
|
@austince these functions are already exported, but when they are overridden for any reason, the changes are not reflected in the library itself. Explicit interfaces for this code will only increase the code complexity as you have guess where to place the handlers and expose an API for these hooks. It is a better code practice to use what you have exported to allow users to add Proxies or override certain behaviors as needed, regardless of my specific motivation to submit this PR. |
|
Hello @Bamieh, thanks for your contribution I think I'm with @austince here, why can't you just write your own little wrapper around this library to accomplish something like that? In general I prefer doing that instead of monkey patch. I'm not sure if we should add this feature and then have to maintain it and make sure we don't stop supporting something like that in the future. I'll close this since it's being a long time and I don't think that's a good addition, but if anyone else in @chaijs/chai-http disagrees we can reopen and continue the discussion. If for some reason we decide to support this this PR looks good, I would just ask you to add tests so we can make sure you never stop supporting it by mistake. |
|
The methods are ALREADY exported by the library (https://github.com/chaijs/chai-http/blob/master/lib/request.js#L251-L253). This is a BUG fix not a feature. These exported methods are not used internally by your library, so right now it is useless to export them, since any modification on them is useless. You will not have to support any additional thing because you already export them, and you do not have to add any tests neither because this is a Node.js behavior for the IMHO this is what would users using these exported methods expect as it is consistent and in line with what other libraries do as well in the node ecosystem. This is a year and a half PR. I don't want to reopen another 2 year discussion over a minor bug fix. |
In the
/lib/request.jsmodule, these methods are exported:Exporting these methods gives the benefit for those who need extra functionality to extend these methods as needed. However, the inner code does not use these exported modules, rather it uses the private ones (ie
Testdirectly) this handicapes the feature, as extending the exported methods practically does nothing.The proposed implementation can be seen on major code bases such as
nodeandmocha; so i believe its a better practice over all.P.S. i needed to override
Testmodule to add extra logging on each request sent, but overriding the Test method does nothing.