feat(@embark/test-runner): introduce artifacts.require API

[0x-r4bbit]

This commit adds a convenience API artifacts.require(name) that aims to make
requiring artifacts a little bit more straight forward.

Usage:

const SimpleStorage = artifacts.require('SimpleStorage');
const EmbarkJS = artifacts.require('EmbarkJS');

What did you refactor, implement, or fix?

How did you do it?

Is there anything that needs special attention (breaking changes, etc)?

Questions

Review

Cool Spaceship Picture

[ghost]

DeepCode's analysis on #ea7f02 found:

• ❌ 0 critical issues. ⚠️ 0 warnings and 1 minor issue. ✔️ 0 issues were fixed.

💬 This comment has been generated by the DeepCode bot, installed by the owner of the repository. The DeepCode bot protects your repository by detecting and commenting on security vulnerabilities or other critical issues.

---

☺️ If you want to provide feedback on our bot, here is how to contact us.

[jrainville]

Lol, we've come full circle

[michaelsbradleyjr]

Embark is a special alias that's only setup in the context of a dapp when it's using the legacy webpack pipeline:

• embark/packages/plugins/basic-pipeline/src/webpack.config.js

  Lines 143 to 145 in 3df75f1

  	'babel-plugin-module-resolver', {
  	alias: embarkAliases
  	}

• embark/packages/plugins/basic-pipeline/src/index.js

  Lines 52 to 53 in 3df75f1

  	(next) => {
  	importsList["Embark/EmbarkJS"] = dappPath(self.embarkConfig.generationDir, 'embarkjs.js');

I'm confused re: how this is supposed to work because isn't mocha-tests something totally independent from the legacy pipeline?

[jrainville]

Yes, but the tests have their own require override https://github.com/embarklabs/embark/blob/master/packages/plugins/mocha-tests/src/lib/index.js#L188-L211

[michaelsbradleyjr]

Thanks @jrainville! I had forgotten about that, will unmark as "changes requested".

In the bigger scheme of things, I think this kind of patching is problematic, though it works for now and that's great. It's "magic" that goes against the idea of Embark being agnostic with respect to other tools, whether they be front-end build tools or test runners, etc. I think part of the future re-think of EmbarkJS needs to carefully consider the relationship between artifact/scaffolding generation and the range of ways users may need/want to import the generated modules: require, import, import().

[0x-r4bbit]

It's "magic" that goes against the idea of Embark being agnostic with respect to other tools, whether they be front-end build tools or test runners, etc

@michaelsbradleyjr I fully agree with what you say. Have the same concerns and raised them a couple of times in the past. However it seems that for now the main focus it to ensure interoperability/easy migration with other tools.

[michaelsbradleyjr]

A few more thoughts on this, and maybe @jrainville can weigh in with respect to the relevant code in mocha-tests.

The use of "Embark/contracts/" in mocha-tests seems to have been for parity with the aliases setup in the legacy pipeline.

While removing the Module.prototype.require hack would be a breaking change — since dapps would no longer be able to manually require('Embark/contracts/[contract]') — maybe we have the opportunity to create a deprecation path.

What if an artifactsRequire function were created in an outer scope (or maybe as a static method on class MochaTestRunner). Then global.artifacts.require could simply be a reference to artifactsRequire; and Module.prototype.require could have logic that checks for Embark/contracts and Embark/EmbarkJS paths and in those cases call to artifactsRequire but warn the user that Embark/ paths are deprecated and in the future global artifacts.require() should be used instead.

[jrainville]

It should be doable. I don't mind. Depends what @iurimatias has in mind.
iirc, in the 4.0 breaking changes, we had that we no longer supported magic requires, but then we added them back 🤷‍♂ (confirmed here: https://framework.embarklabs.io/docs/migrating_from_3.x.html#Updating-%E2%80%9Cmagic%E2%80%9D-EmbarkJS-imports)

[michaelsbradleyjr]

Just to be clear. What I'm suggesting above is that since the magic requires are available to users currently, we shouldn't break them in v5. But we can move toward isolating them to the legacy webpack pipeline, since they're not in any way needed for a global artifacts.require to work as desired.

[0x-r4bbit]

Updated the PR to make CI happy. @michaelsbradleyjr I've also added an inline comment to avoid confusion for the next person looking at this code.

Thanks for the review!

[emizzle]

I agree that we should create a deprecation path for magic imports, however, I don't think that needs to be addressed in this PR as the scope of changes is larger (would affect dapps and tests, whereas this PR affects only tests) and more considerations needs to be taken in to account.

[michaelsbradleyjr]

Here's a comparison between this PR's branch and a branch that implements what I proposed in earlier comments:

feat/artifact-require...feat/artifact-require-mb

In addition to changes in packages/plugins/mocha-tests/src/lib/index.js, the dapp templates and tests have been updated to use the artifacts.require() syntax.

If this PR were updated to include the changes I'm proposing and then included in Embark v5.2.0, when embark test is run in dapps still using the "magic prefix" requires (in tests), a deprecation warning will be printed. See the screenshot below (it's the demo modified to still use the prefix in its /test/simple_storage_spec.js):

In Embark v6.0.0 (or a later major version) we would then completely remove the monkey-patching of Module.prototype.require in packages/plugins/mocha-tests/src/lib/index.js.

Note that this deprecation path for the monkey-patched Module.prototype.require is completely independent of use of "magic prefixes" in dapp sources that are built using our legacy basic-pipeline. Case in point, the prefixes have not been removed from the import statements in .js files in dapps/templates/demo/app/ and they still work correctly.

[iurimatias]

IMO we should go with @michaelsbradleyjr changes

[0x-r4bbit]

@iurimatias rebased this with @michaelsbradleyjr changes