chore: make contract artifacts usable in node #1632

andremedeiros · 2019-05-21T18:38:37Z

This will work 100% of the time, all the time.

jrainville · 2019-05-21T19:00:18Z

packages/embark-code-generator/src/index.js

+
+      let ${contractName}JSONConfig = ${JSON.stringify(contractJSON)};
+      let ${contractName} = new EmbarkJS.Blockchain.Contract(${contractName}JSONConfig);
+      export default ${contractName};


I think this is where problems might arise. export doesn't exist in Node. I forgot to mention it to you because I hadn't notice because the problem the user was having was with the import.
When trying to do the node embarkjs, it's the export that gave me the most problems, because you can't put export in an if... Maybe you can find a solution, but maybe we'll need to transpile those too 😞

Wait, does module.exports not work on the browser?

Last I tried, it didn't. Maybe it was something I was doing wrong.

jrainville

Looks good (haven't tested, but I assume it works)

ghost · 2019-05-23T18:02:17Z

DeepCode analyzed this pull request.
There is 1 new info report.

Click to see more details.

ghost · 2019-05-23T18:05:26Z

DeepCode analyzed this pull request.
There are no new issues. 1 info report was fixed.

Click to see more details.

ghost · 2019-05-23T18:10:14Z

DeepCode analyzed this pull request.
There are no new issues. 1 info report was fixed.

Click to see more details.

jrainville

Very nice!
Would just be better to use constants

jrainville · 2019-05-23T18:26:19Z

packages/embark-whisper/src/index.js

@@ -24,6 +24,7 @@ class Whisper {
    this.embark = embark;
    this.web3Ready = false;
    this.webSocketsChannels = {};
+    this.modulesPath = dappPath(embark.config.embarkConfig.generationDir + "/modules");


/modules should be put in a constant and used everywhere from the symlink generation to the uses in modules

Suggested change

this.modulesPath = dappPath(embark.config.embarkConfig.generationDir + "/modules");

this.modulesPath = dappPath(embark.config.embarkConfig.generationDir, "modules");

Also should make use of the path-joining that dappPath() does because doing + "/" is problematic on Windows.

jrainville · 2019-05-23T18:26:25Z

packages/embark/src/lib/modules/ipfs/index.js

@@ -23,6 +23,7 @@ class IPFS {
    this.addedToConsole = false;
    this.storageProcessesLauncher = null;
    this.usingRunningNode = false;
+    this.modulesPath = dappPath(embark.config.embarkConfig.generationDir + "/modules");


jrainville · 2019-05-23T18:26:41Z

packages/embark/src/lib/modules/swarm/index.js

@@ -23,6 +23,7 @@ class Swarm {
    this.addedToConsole = false;
    this.storageProcessesLauncher = null;
    this.usingRunningNode = false;
+    this.modulesPath = dappPath(embark.config.embarkConfig.generationDir + "/modules");


michaelsbradleyjr · 2019-05-23T19:24:29Z

packages/embark-whisper/src/index.js

@@ -24,6 +24,7 @@ class Whisper {
    this.embark = embark;
    this.web3Ready = false;
    this.webSocketsChannels = {};
+    this.modulesPath = dappPath(embark.config.embarkConfig.generationDir + "/modules");


Suggested change

this.modulesPath = dappPath(embark.config.embarkConfig.generationDir + "/modules");

this.modulesPath = dappPath(embark.config.embarkConfig.generationDir, "modules");

Also should make use of the path-joining that dappPath() does because doing + "/" is problematic on Windows.

jrainville

🙌

michaelsbradleyjr

Don't merge yet: tests are consistently not passing on Windows, probably some path-related thing. I'm investigating currently and will commit on this branch when I've fixed it.

michaelsbradleyjr · 2019-05-26T21:42:32Z

What was needed for this to work on Windows:

https://github.com/embark-framework/embark/blob/d855fa1d61a105540c0c59a29c5e02e135038905/packages/embark-whisper/src/index.js#L130

https://github.com/embark-framework/embark/blob/d855fa1d61a105540c0c59a29c5e02e135038905/packages/embark/src/lib/modules/ipfs/index.js#L150

https://github.com/embark-framework/embark/blob/d855fa1d61a105540c0c59a29c5e02e135038905/packages/embark/src/lib/modules/swarm/index.js#L130

Because we're using the path-strings in the context of code generation, the \ characters need to be double-escaped.

michaelsbradleyjr · 2019-05-26T21:50:33Z

Please see: https://github.com/embark-framework/embark/blob/chore/make-contract-artifacts-usable-in-node/packages/embark/src/cmd/cmd_controller.js#L676-L678

The reason that's needed is that otherwise symlink creation for embarkjs-ipfs|swarm|whisper will be in a race with the startup of other services.

The problem isn't noticed after the first run of embark test, but if you do embark reset before embark test, or test a fresh dapp, (without this change in place) there will be errors re: missing modules, e.g. embarkjs-swarm.

Unfortunately, it's not just embark test that needs to be changed. A similar fix is needed for embark run|build|etc, basically any command that involves engine.startService("codeGenerator");.

Rather than make those adjustments now, I'm leaving this PR marked as changes requested, and I think we should discuss if the fix above is the way to go or if there's a better way.

michaelsbradleyjr · 2019-05-28T17:56:35Z

These are all the places the workaround mentioned in my previous comment have been introduced:

https://github.com/embark-framework/embark/blob/chore/make-contract-artifacts-usable-in-node/packages/embark/src/cmd/cmd_controller.js#L168-L170

https://github.com/embark-framework/embark/blob/chore/make-contract-artifacts-usable-in-node/packages/embark/src/cmd/cmd_controller.js#L245-L247

https://github.com/embark-framework/embark/blob/chore/make-contract-artifacts-usable-in-node/packages/embark/src/cmd/cmd_controller.js#L332-L334

https://github.com/embark-framework/embark/blob/chore/make-contract-artifacts-usable-in-node/packages/embark/src/cmd/cmd_controller.js#L416-L418

https://github.com/embark-framework/embark/blob/chore/make-contract-artifacts-usable-in-node/packages/embark/src/cmd/cmd_controller.js#L594-L596

https://github.com/embark-framework/embark/blob/chore/make-contract-artifacts-usable-in-node/packages/embark/src/cmd/cmd_controller.js#L691-L693

Tests are passing, and I successfully test-drove each of the commands that had the change introduced.

ghost · 2019-05-28T18:26:17Z

DeepCode encountered a problem when analyzing this pull request. If you want to retry, create a comment: "Retry Deepcode".

michaelsbradleyjr · 2019-06-01T22:33:55Z

Retry Deepcode

ghost · 2019-06-01T22:33:56Z

DeepCode analyzed this pull request.
There are no new issues. 1 info report was fixed.

Click to see more details.

andremedeiros requested a review from a team May 21, 2019 18:38

jrainville reviewed May 21, 2019

View reviewed changes

andremedeiros force-pushed the chore/make-contract-artifacts-usable-in-node branch 2 times, most recently from a0a3554 to ccb8fb0 Compare May 21, 2019 20:49

jrainville approved these changes May 22, 2019

View reviewed changes

andremedeiros force-pushed the chore/make-contract-artifacts-usable-in-node branch from ccb8fb0 to 1f336f2 Compare May 23, 2019 18:02

andremedeiros force-pushed the chore/make-contract-artifacts-usable-in-node branch from 1f336f2 to a3ee37a Compare May 23, 2019 18:05

andremedeiros force-pushed the chore/make-contract-artifacts-usable-in-node branch 2 times, most recently from 25918e8 to 5618c49 Compare May 23, 2019 18:10

michaelsbradleyjr approved these changes May 23, 2019

View reviewed changes

jrainville requested changes May 23, 2019

View reviewed changes

andremedeiros force-pushed the chore/make-contract-artifacts-usable-in-node branch from 5618c49 to 52ea0aa Compare May 23, 2019 18:41

michaelsbradleyjr suggested changes May 23, 2019

View reviewed changes

andremedeiros force-pushed the chore/make-contract-artifacts-usable-in-node branch from 52ea0aa to cc493f1 Compare May 23, 2019 19:48

andremedeiros requested review from michaelsbradleyjr and jrainville May 23, 2019 19:48

andremedeiros force-pushed the chore/make-contract-artifacts-usable-in-node branch from cc493f1 to 0827ad4 Compare May 23, 2019 19:54

jrainville approved these changes May 23, 2019

View reviewed changes

michaelsbradleyjr approved these changes May 23, 2019

View reviewed changes

michaelsbradleyjr suggested changes May 24, 2019

View reviewed changes

michaelsbradleyjr force-pushed the chore/make-contract-artifacts-usable-in-node branch 3 times, most recently from 55260a5 to d855fa1 Compare May 26, 2019 21:16

chore: make contract artifacts usable in node

3a19f10

michaelsbradleyjr force-pushed the chore/make-contract-artifacts-usable-in-node branch from d855fa1 to 3a19f10 Compare May 28, 2019 17:27

michaelsbradleyjr approved these changes May 28, 2019

View reviewed changes

michaelsbradleyjr mentioned this pull request May 28, 2019

refactor (@embark/embarkjs-ens): move embarkjs-ens to its own module #1562

Merged

michaelsbradleyjr merged commit 0f633cd into master Jun 1, 2019

delete-merged-branch bot deleted the chore/make-contract-artifacts-usable-in-node branch June 1, 2019 22:35

michaelsbradleyjr mentioned this pull request Jun 5, 2019

fix(@embarkjs): unconditionally require symlinked embarkjs-* modules #1651

Merged

michaelsbradleyjr mentioned this pull request Jun 17, 2019

refactor(@embarkjs/web3): make web3 connector an internal plugin like the other embarkjs-* packages #1661

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: make contract artifacts usable in node #1632

chore: make contract artifacts usable in node #1632

andremedeiros commented May 21, 2019 •

edited

Loading

jrainville May 21, 2019

andremedeiros May 21, 2019

jrainville May 21, 2019

jrainville left a comment

ghost commented May 23, 2019

ghost commented May 23, 2019

ghost commented May 23, 2019

jrainville left a comment

jrainville May 23, 2019

michaelsbradleyjr May 23, 2019

jrainville May 23, 2019

jrainville May 23, 2019

michaelsbradleyjr May 23, 2019

jrainville left a comment

michaelsbradleyjr left a comment

michaelsbradleyjr commented May 26, 2019 •

edited

Loading

michaelsbradleyjr commented May 26, 2019 •

edited

Loading

michaelsbradleyjr commented May 28, 2019

ghost commented May 28, 2019

michaelsbradleyjr commented Jun 1, 2019

ghost commented Jun 1, 2019

	this.modulesPath = dappPath(embark.config.embarkConfig.generationDir + "/modules");
	this.modulesPath = dappPath(embark.config.embarkConfig.generationDir, "modules");

chore: make contract artifacts usable in node #1632

chore: make contract artifacts usable in node #1632

Conversation

andremedeiros commented May 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrainville left a comment

Choose a reason for hiding this comment

ghost commented May 23, 2019

ghost commented May 23, 2019

ghost commented May 23, 2019

jrainville left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrainville left a comment

Choose a reason for hiding this comment

michaelsbradleyjr left a comment

Choose a reason for hiding this comment

michaelsbradleyjr commented May 26, 2019 • edited Loading

michaelsbradleyjr commented May 26, 2019 • edited Loading

michaelsbradleyjr commented May 28, 2019

ghost commented May 28, 2019

michaelsbradleyjr commented Jun 1, 2019

ghost commented Jun 1, 2019

andremedeiros commented May 21, 2019 •

edited

Loading

michaelsbradleyjr commented May 26, 2019 •

edited

Loading

michaelsbradleyjr commented May 26, 2019 •

edited

Loading