Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Bug fix: Correctly return value for foo = await bar in console #5270

Merged
merged 2 commits into from
Jul 8, 2022

Conversation

haltman-at
Copy link
Contributor

@haltman-at haltman-at commented Jul 8, 2022

Addresses #3349.

This PR fixes the issue where expressions of the form foo = await bar, when entered in the Truffle Console, would perform the assignment but then not return the resulting value. Note that this PR does not fix the more general issues with the use of await in the Truffle Console, which is issue #1546. Fixing the broader problems are probably going to have to wait until we have removed support for both Node 12 and Node 14.

The fundamental problem, and the reason for #1546, is that all our await-handling code in the console is a bunch of hacks. The ultimate solution to all of this, once we don't need to support Node 12 and Node 14 anymore, will be to remove all these hacks and do things the straightforward way that Node 16 will allow us to do it. However, in the meantime, I still wanted to fix the particular issue of #3349. To accomplish that, this PR uses... more hacks. But, it's not worse than the already existing hacks surrounding it, so I think it's fine.

The reason that we get undefined in this case is because we have special handling for this particular case, and that special handling involves creating a little assignment script and then passing it to a VM to run; we do get the return value from our script, but the problem is that we wrote the script in a way such that it always returns undefined. So, I've modified it to (conditionally) return the assigned value instead.

Specifically, if we are doing a bare assignment (so no let or const, those shouldn't return values), I modify the assignment script by appending the variable name to the end of it, so that the value is returned. This solves the problem! Or the narrow problem of #3349, anyway. The broad problem of #1546 will have to wait...

(Although, you can do things beyond just foo = await bar with it... e.g., x = (await web3.eth.net.getId()) + 1 will work fine!)

I didn't include any tests in this PR because um I'm not sure how to test this programmatically. But potentially tests could be added if people think it's necessary (and if anyone can give me an idea for how, or point me to where we already do this sort of thing...)

Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one @haltman-at! I'll be able to review this tomorrow. This can be tested by utilizing the same setup for console/develop REPL tests.

    assert.equal(
      output.indexOf('undefined'),
      -1,
      "should not print `undefined` on naked assignments"
    );

@haltman-at
Copy link
Contributor Author

Oh, I see! I only looked at console, not develop, so I didn't see that. Cool, I can go back and add a test then! (Of course I won't be abusing indexOf like that, sheesh, we have better ways these days. :P )

Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one Harry. It's a good approach given the constraints we have. Of course, it would be nice to get a few tests in.

packages/core/lib/console.js Show resolved Hide resolved
@haltman-at
Copy link
Contributor Author

OK, added the test!

Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Nice work @haltman-at

@haltman-at haltman-at merged commit 336bbac into develop Jul 8, 2022
@haltman-at haltman-at deleted the show-me-the-value branch July 8, 2022 18:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants