Fix #253 Cannot read property 'then' of null in Transport.js issue. #254
Conversation
alexrayan
force-pushed
the
bugfix/then_on_null_in_transport_error_fix
branch
from
e7b54f4 to
b863344
Compare
|
Trying to reproduce and verify the fix here, do see the issue. Here is the detail . |
test/unit/client.test.js
Outdated
| } | ||
| }) | ||
|
|
||
| test('Issue #1521 with callbacks', t => { |
There was a problem hiding this comment.
@ananzh Updated! Thank you :)
Sorry for the delay with the response. I am able to see the test failing on "Issue #253 with callbacks" in my local without the code change and the test passing with the code fix.
We actually encountered this issue in our own production flow which is using callbacks when we switched to ES version 7 from ES version 5.
There was a problem hiding this comment.
Okay. Not a problem. I think making lib more robust is a must. Meanwhile I do see error msg from unit tests. Thanks for the contribution.
There was a problem hiding this comment.
Agree, hopefully I can contribute more if time permits. Looks like a lot of people are moving towards the open source distribution of the ES.
Do you see any other error msgs in the unit tests? All the tests are passing on my branch. Or did you mean that the null type error is triggering in unit tests without the fix in place?
Would love to merge this fix to the opensearch-project main repo and switch off of our fork. :)
…nsport.js issue. Signed-off-by: aizaguirre <[email protected]>
Signed-off-by: aizaguirre <[email protected]>
alexrayan
force-pushed
the
bugfix/then_on_null_in_transport_error_fix
branch
from
59263f5 to
19a431b
Compare
|
this fix our issue where the opensearch client is throwing errors on every requests |
|
@ananzh Are these any plans to merge this PR to the code base? Looks like it requires more than 1 approval |
There was a problem hiding this comment.
Hello @alexrayan,
Thanks for doing this! The changes look good but one thing to note is that it would appear the code is copied from the fork after the licensing change.
For the source code there isn't much you can but the tests has references to Elasticsearch. Could we clean up these tests to avoid any issues with licensing.
Thank you again!
Hi @kavilla! Sure, let me cleanup the tests and remove the refs shortly. |
|
@alexrayan Want to finish this? |
|
@alexrayan thanks for this 🙏🏻 cleaned up the unit tests
|
|
if that helps I rebased the commits to remove any ref to the elasticsearch codebase in #283 |
|
@rawpixel-vincent Thank you for that! Appreciate it. Apologies for the delay. Life and work got in the way so wasn't able to find time to clean up the tests to remove the ES references. |
|
Implemented with the following PR https://github.com/opensearch-project/opensearch-js/pull/283/files, so closing mine. |
Description
Porting a fix for "Cannot read property 'then' of null in Transport.js issue".
based on the original fix for the same issue in elastic/elasticsearch-js client lib here: elastic/elasticsearch-js#1594
Issues Resolved
Fix #253
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.