fix: empty-node-fetch-response #58
Conversation
Signed-off-by: Shreyansh Shah <[email protected]>
|
@re-Tick Can you please review this? @shreyanshshah27 Please wait while the maintainers review this. After that you can add this in the completed task list in the gsoc repo! |
integrations/node-fetch/require.ts
Outdated
| @@ -37,6 +38,9 @@ export function wrappedNodeFetch(fetch: any) { | |||
| url: any, | |||
| options: any | |||
| ) { | |||
| if (process.env.KEPLOY_MODE == MODE_OFF) { | |||
There was a problem hiding this comment.
the context should be created in middleware. here we can do an actual fetch call along console in below if block
if (
getExecutionContext() == undefined ||
getExecutionContext().context == undefinedThere was a problem hiding this comment.
Hey @re-Tick
It makes sense not to create a context here and directly calling the fetch function. I've incorporated this change, but a bit differently.
There was a problem hiding this comment.
If the user have not provided the KEPLOY_MODE env variable then, it will still do the same problem.
There was a problem hiding this comment.
That is correct. but probably that is the ideal behavior right? If the user isn't providing the KEPLOY_MODE then it's better to return some error and inform the user, instead of directly assuming OFF mode.
There was a problem hiding this comment.
Yes we should console an error to inform the user but also we should do the actual function call so, that the user application do not break due to Keploy SDK integration
There was a problem hiding this comment.
That is correct. but probably that is the ideal behavior right? If the user isn't providing the KEPLOY_MODE then it's better to return some error and inform the user, instead of directly assuming OFF mode.
The default value of KEPLOY_MODE is "off". So, if the env variable is empty then, user application should run on off mode.
Here, the issue is that when user have missed the router integration in its application, then the context in async map is empty. The behaviour should be to log an error message and return the outputs of the actual wrapped function call.
There was a problem hiding this comment.
Okay. Understood. My apologies, I wasn't aware that default KEPLOY_MODE is "off". In that case, what you are suggesting, makes sense. I've made the required changes. @re-Tick Can you please review this again?
re-Tick
added
Don't Merge
Signed-off-by: Shreyansh Shah <[email protected]>
shreyanshshah27
force-pushed
the
fix-empty-node-fetch-response
branch
from
461512b
to
5f014eb
Compare
re-Tick
removed
Don't Merge
Signed-off-by: Shreyansh Shah <[email protected]>
|
@re-Tick @developer-diganta Can someone please review this PR? |
|
LGTM |
re-Tick
added
Don't Merge
This fixes keploy/keploy#376