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

Set context vars #3317

Merged
merged 10 commits into from
Sep 1, 2020
Merged

Conversation

fainashalts
Copy link
Contributor

@fainashalts fainashalts commented Aug 27, 2020

This PR seeks to allow the re-introduction of the child process for commands from #3255, the spawn-repl branch, while fixing a few issues:

  1. A bug where Artifacts were not available within the truffle develop and truffle console commands. It appears the context containing the contracts' artifacts was not properly being set for the new repl.

To test that this bug is fixed:
a) Run truffle console or truffle develop in an existing project that has already migrated its contracts.
b) Enter the name of one of its contracts.
Expected: The artifact for that contract is printed in your terminal.

a) Run truffle console or truffle develop in an existing project for which a migration has not been run.
b) Enter the name of a contract in the project.
Expected: Error thrown
c) Run the migrate command
d) Enter the name of a contract in the project
Expected: The artifact is printed in your terminal.

  1. This PR also updates the stdio handling of stderr in the spawn process, to keep warnings that have already been consoled in the parent process from being printed again. Specifically, this is in relation to the error about having two config files:
Warning: Both truffle-config.js and truffle.js were found. Using truffle-config.js.
  1. This PR also pull this this.provision() function call outside of the try/catch it was in, to ensure that if there is an error in that function that we are able to see it.

  2. This PR also fixes the use of yargs for properly getting and passing the network in the console-child file. (good catch on this bug @cds-amal!)

  3. This work additionally ensures that assignments/scripts created in Javascript inside the child process persist between commands. You should be able to run const [ownerAccount] = accounts and access ownerAccount throughout your develop or console session.

Many thanks to @cds-amal for letting me bounce ideas off of him while figuring out this bug!

@fainashalts fainashalts changed the base branch from develop to revert-3314-revert-3255-spawn-repl August 27, 2020 02:12
@fainashalts fainashalts requested review from cds-amal and gnidan August 27, 2020 02:20
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 find on the yargs @fainashalts . I have a few comments on the context logic.

@fainashalts fainashalts requested a review from cds-amal August 28, 2020 02:50
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.

The interpret method duplicates logic of runSpawn that can be refactored out.

@@ -118,7 +122,6 @@ class Console extends EventEmitter {
});

this.resetContractsInConsoleContext(abstractions);
Copy link
Member

Choose a reason for hiding this comment

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

Would you consider renaming this function to addContractReferencesToReplContext?

Copy link
Contributor Author

@fainashalts fainashalts Aug 28, 2020

Choose a reason for hiding this comment

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

I think this is a reset and not an addition. It happens at the end of every command, and if the contracts have changed they get reset to the new ones. Happy to discuss further but this was the original name of this function in the replManager and I think it is still apt.

Copy link
Member

Choose a reason for hiding this comment

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

The content of the function mutates repl.context instead of a Console. Not a hill I want to stop on :)

@fainashalts fainashalts requested a review from cds-amal August 28, 2020 22:29
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 work @fainashalts :) Sorry about the Object Spread usage!

@@ -118,7 +122,6 @@ class Console extends EventEmitter {
});

this.resetContractsInConsoleContext(abstractions);
Copy link
Member

Choose a reason for hiding this comment

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

The content of the function mutates repl.context instead of a Console. Not a hill I want to stop on :)

@fainashalts fainashalts merged commit dc5bc5f into revert-3314-revert-3255-spawn-repl Sep 1, 2020
@fainashalts fainashalts deleted the set-context-vars branch September 1, 2020 17:14
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