Implement isInjected read-only request property to indicate request…#4117
Implement isInjected read-only request property to indicate request…#4117cjihrig merged 8 commits intohapijs:masterfrom
isInjected read-only request property to indicate request…#4117Conversation
… created by `server.inject`.
b89f059 to
8686b67
Compare
Implemented. |
|
Can you guys take a look / merge #4125 to address the 2 failing unit tests on master? I'll rebase this one when it lands Nevermind, issue locally was a combination of me being on the VPN, and as @cjhrig explained in the other thread:
So build for this PR should be green with a fresh re-build. |
| const internals = { | ||
| events: Podium.validate(['finish', { name: 'peek', spread: true }, 'disconnect']), | ||
| reserved: ['server', 'url', 'query', 'path', 'method', 'mime', 'setUrl', 'setMethod', 'headers', 'id', 'app', 'plugins', 'route', 'auth', 'pre', 'preResponses', 'info', 'orig', 'params', 'paramsArray', 'payload', 'state', 'jsonp', 'response', 'raw', 'domain', 'log', 'logs', 'generateResponse'] | ||
| reserved: ['server', 'url', 'query', 'path', 'method', 'mime', 'setUrl', 'setMethod', 'headers', 'id', 'app', 'plugins', 'route', 'auth', 'pre', 'preResponses', 'info', 'isInjected', 'orig', 'params', 'paramsArray', 'payload', 'state', 'jsonp', 'response', 'raw', 'domain', 'log', 'logs', 'generateResponse'] |
There was a problem hiding this comment.
This could arguably be seen as a breaking change. Maybe this should go into v20 for good measure CC @cjihrig.
There was a problem hiding this comment.
I don't think we should consider this semver major since the request object is owned by hapi. Any user code that is attaching things to the request object can use a symbol, request.app, etc.
There was a problem hiding this comment.
I was thinking of server.decorate('request', ...) usage, but either way I see your point. For example, the schmervice plugin adds support for request.services(). If hapi decided to name something "services" then it would break that plugin.
There was a problem hiding this comment.
What you're saying makes sense. One thing we could do: automatically consider any additions to the reserved lists across the codebase as semver major.
Another longer term, but not mutually exclusive, idea: have decorate() update some new object (request.decorations?) on requests so that they won't ever conflict with hapi.
devinivy
left a comment
There was a problem hiding this comment.
I believe the build breakage is related directly to hapijs/lab#981. Beyond that this looks good to me. I made a note, but it may make sense to include this as a small feature in v20 given that reserving request.isInjection could be seen as a breaking change. It's pretty slight, though.
|
I cleared the Travis cache. Hopefully the CI will be green now. |
Co-authored-by: Colin Ihrig <cjihrig@gmail.com>
298f11f to
ce1367e
Compare
Co-authored-by: Colin Ihrig <cjihrig@gmail.com>
This reverts commit 674f3be.
|
Thanks for the contribution! |
Closes #4116
Implements
isInjectedread-only request property to indicate request is injected byserver.inject()