Added another function to Table that will process async callback func… #571
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tions to resolve issue getgauge#570. Also had to bump the ecmaVersion to 8 in eslint to support async await Signed-off-by: Autumn Calhoun <autumn.calhoun@crowncastle.com>
zabil
reviewed
Apr 20, 2023
src/table.js
Outdated
| @@ -8,6 +8,14 @@ var Table = function (protoTable) { | |||
| callback(entry); | |||
| } | |||
| }; | |||
|
|
|||
| this.asyncEntries = async function (callback) { | |||
There was a problem hiding this comment.
Instead of making a new function, can we just check if the callback is an async function and handle it accordingly?
For example
const AsyncFunction = (async () => {}).constructor;
if(callback instanceof AsyncFunction) {
}
There was a problem hiding this comment.
@zabil Thank you for the suggestion, I am new to javascript, so that is helpful. I have made the suggested changes.
Signed-off-by: Autumn Calhoun <autumn.calhoun@crowncastle.com>
|
Thanks for the change @autumn-calhoun! |
|
@zabil I see the build for the merge to master failed. I don't see a way to rerun it, and it looks like the failed test has nothing to do with my changes. What do I need to do to resolve this? |
|
The build is unfortunately failing for some other reason. It's been flaky in the past. I am gonna spend some time looking at it. Will do it next week. |
|
@zabil Are there any updates on the build? |
|
@zabil Sorry to keep bothering you, but we would like to be able to utilize this fix. Do you have any update on the build? |
|
Hi @autumn-calhoun - managed to fix the build and release as 3.0.0 (along with some dependency security fixes) |
|
Thank you so much Chad! I was able to install the new version, however, for some reason it does not work in our environment. I’m not sure if it’s because we are running a different version of node.js. But I made another small change and tested it both with my use case and the gauge unit tests. Are you able to review that PR?
#576
From: Chad Wilson ***@***.***>
Sent: Saturday, July 8, 2023 12:16 PM
To: getgauge/gauge-js ***@***.***>
Cc: Calhoun, Autumn ***@***.***>; Mention ***@***.***>
Subject: Re: [getgauge/gauge-js] Added another function to Table that will process async callback func… (PR #571)
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
Hi @autumn-calhoun<https://github.com/autumn-calhoun> - managed to fix the build and release as 3.0.0
—
Reply to this email directly, view it on GitHub<#571 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AN74Q2YWL7A3B55ZBDLGMM3XPGBUJANCNFSM6AAAAAAW6JYLEE>.
You are receiving this because you were mentioned.Message ID: ***@***.******@***.***>>
This email may contain confidential or privileged material. Use or disclosure of it by anyone other than the recipient is unauthorized. If you are not an intended recipient, please delete this email.
|
|
Is there a test that fails in your environment on 3.0.0? What is the OS and node version you are running? Did the version as of your PR here work in your env earlier or you are not sure? (Want to figure out if any of the dependency bumps I made regressed something for you - don't think so, but want to check) It's probably best that we understand what is going on to avoid it being broken again. |
|
The part that is failing was the new change I added to 3.0.0 to handle async callback methods in the table entries function. When I made that change, I didn’t think to generate the package locally and install that into my project to test it, I just relied on the unit tests. We are running on Windows 11/ Node.js 18.12. When I run the table.entries method in our test project the below code keeps returning false, even though callback.constructor.name === “AsyncFunction” returns true.
const AsyncFunction = (async () => {}).constructor;
callback instanceof AsyncFunction
I tried to do some research on why one way worked and the other does not, but I wasn’t able to figure out the exact reason. I tried declaring my async function a couple different ways, but callback instanceof AsyncFunction always returns false.
From: Chad Wilson ***@***.***>
Sent: Monday, July 10, 2023 11:04 AM
To: getgauge/gauge-js ***@***.***>
Cc: Calhoun, Autumn ***@***.***>; Mention ***@***.***>
Subject: Re: [getgauge/gauge-js] Added another function to Table that will process async callback func… (PR #571)
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
Is there a test that fails in your environment on 3.0.0? What is the OS and node version you are running?
Did the version as of your PR here work in your env earlier or you are not sure? (Want to figure out if any of the dependency bumps I made regressed something for you - don't think so, but want to check)
It's probably best that we understand what is going on to avoid it being broken again.
—
Reply to this email directly, view it on GitHub<#571 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AN74Q2ZOET2LCMB2JB4Q5XLXPQKXDANCNFSM6AAAAAAW6JYLEE>.
You are receiving this because you were mentioned.Message ID: ***@***.******@***.***>>
This email may contain confidential or privileged material. Use or disclosure of it by anyone other than the recipient is unauthorized. If you are not an intended recipient, please delete this email.
|
|
OK, your change in #576 doesn't seem to make anything worse anyway. Do you perhaps have babel or similar in your "real" environment, maybe transforming asyncs to generator functions or something like that which breaks the earlier test? |
|
Not that I am aware, but that is the one thing I found that could explain the issue.
From: Chad Wilson ***@***.***>
Sent: Monday, July 10, 2023 11:20 AM
To: getgauge/gauge-js ***@***.***>
Cc: Calhoun, Autumn ***@***.***>; Mention ***@***.***>
Subject: Re: [getgauge/gauge-js] Added another function to Table that will process async callback func… (PR #571)
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
OK, your change doesn't seem to make anything worse anyway.
Do you perhaps have babel or similar in your "real" environment, maybe transforming asyncs to generator functions or something like that which breaks the earlier test?
—
Reply to this email directly, view it on GitHub<#571 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AN74Q25WBJGZKXPM7Q6D4JDXPQMTLANCNFSM6AAAAAAW6JYLEE>.
You are receiving this because you were mentioned.Message ID: ***@***.******@***.***>>
This email may contain confidential or privileged material. Use or disclosure of it by anyone other than the recipient is unauthorized. If you are not an intended recipient, please delete this email.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…tions to resolve issue #570. Also had to bump the ecmaVersion to 8 in eslint to support async await