-
Notifications
You must be signed in to change notification settings - Fork 61
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
feat: add support to skip request execution from script. #942
Conversation
38e4243
to
4c4c586
Compare
2da5532
to
323de69
Compare
73216f7
to
d442bcf
Compare
npm/build-sandbox-types.js
Outdated
@@ -129,6 +129,20 @@ module.exports = function (exit) { | |||
c.type.typeName.escapedText = `import("postman-collection").${currentType}`; | |||
} | |||
} | |||
|
|||
// For properties referencing sdk and some more properties, eg. request: Request|IRequest | |||
if (c.type && c.type.types && c.type.types.length) { |
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.
This will happen if there is c.types.types.types
. Shouldn't this be recursive?
lib/sandbox/dispatch-event.js
Outdated
return; | ||
} | ||
|
||
bridge.dispatch(event, ...args); |
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.
What're your thoughts on overriding the dispatch
method itself? Would solve the problem where consumers can accidentally call .dispatch
directly on the bridge.
const originalDispatch = bridge.dispatch;
bridge.dispatch = function (execution, ...args) {
if (!isExecution(execution)) {
throw new Error('must be called with execution');
}
if (execution.shouldSkipExecution) {
return;
}
originalDispatch.call(bridge, ...args);
};
3424bd3
to
6bf59fd
Compare
6bf59fd
to
a5fea93
Compare
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## develop #942 +/- ##
===========================================
+ Coverage 60.64% 60.78% +0.14%
===========================================
Files 12 12
Lines 559 561 +2
Branches 135 135
===========================================
+ Hits 339 341 +2
Misses 220 220
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
lib/sandbox/execute.js
Outdated
// because this event should be fired even if shouldSkipExecution is true as this event is | ||
// used to complete the execution in the sandbox. All other events are fired only if | ||
// shouldSkipExecution is false. | ||
(dnd !== true) && bridge.dispatch({ shouldSkipExecution: false }, |
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.
We're leaking the implementation where execution can contain shouldSkipExecution
.
declare interface Execution { | ||
/** | ||
* Halts the execution of current request. No line after this will be executed and | ||
* if invoked from a pre-request script, the request will not be sent. |
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.
Need to be more clear for the user. Something like, no scripts associated with the current request will be executed after this and if invoked from a pre-request script, the request will not be sent.
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.
No script seems more confusing as even part of current script after this won't be executed is skipRequest invoked.
declare interface Execution { | ||
/** | ||
* Halts the execution of current request. No line after this will be executed and | ||
* if invoked from a pre-request script, the request will not be sent. |
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.
This is prerequest.d.ts file, so you can write the doc assuming "if executed from pre-request script" to be true.
* if invoked from a pre-request script, the request will not be sent. | |
* the request will not be sent. |
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.
It is not separated like that only some part of jsdocs around response are omitted in case of pre-request script
0c2c00e
to
abcb20c
Compare
abcb20c
to
bf0e253
Compare
Closing as we are changing the approach to: #964 |
pm.request.stopExecution
halts the execution of current request. No line after this will be executed and if invoked from a pre-request script, the request will not be sent.