search

adrianmcli / truffle-react

⚛️ A boilerplate Truffle Box project with Create React App for rapid Ethereum Dapp development
46 stars 5 forks source link

Add Windows support #2

Open adrianmcli opened 6 years ago

adrianmcli commented 6 years ago

Unfortunately I don't have a Windows machine, but in order for Windows support to work, we need to make sure that the symlink is created automatically when unboxing happens. Currently, that is done by running an NPM script npm run link-contracts, but since this uses ln -s, it only works on Linux and Mac.

We basically need a "cross-env" way of doing this.

adrianmcli commented 6 years ago

We can probably use this package: https://github.com/charlesguse/run-script-os

adrianmcli commented 6 years ago

I've dived a little deeper in this issue: https://github.com/adrianmcli/truffle-next/issues/21

SvenMeyer commented 6 years ago

What about actually writing the contracts json (SimpleStorage.json) files into a directory within the react app ?

// truffle.js
const fullPathBuildDirectory = `${__dirname}/client/lib/contracts`;

module.exports = {
  contracts_build_directory: fullPathBuildDirectory,
  networks: {
  ...
truffle(develop)> compile --all
Compiling ./contracts/Migrations.sol...
Compiling ./contracts/SimpleStorage.sol...
Writing artifacts to ./client/lib/contracts

found it here: https://github.com/trufflesuite/truffle-migrate/issues/10#issuecomment-402441008

It looks like as if you need to keep (a copy of) Migrations.json in build/contracts, otherwise you will get an an error like

Error: Attempting to run transaction which calls a contract function, but recipient address 0x8cdaf0cd259887258bc13a92c0a6da92698644c0 is not a contract address

But as Migrations.sol / Migrations.json normally never change, this is just a one time action during project setup and thus should not be too much of an issue.

Above change works with all other code unchanged or think about a better place for the json files and change the reference in Web3Container.js as well.

adrianmcli commented 6 years ago

@SvenMeyer Thanks for the suggestion, it's certainly a more elegant workaround. But I'm hesitant to use it because it changes the default behaviour of Truffle's barebones project (i.e. the traditional build/contracts folder will no longer be up-to-date).

I've also considered another solution, which is to eject the React app and remove the restriction from requiring the JSON files outside of src.

I really think there is no good solution right now, we'll have to see what else we can figure out from this going forward. What do you think about being able to pass an array into the Truffle options so that it could write the artifacts to multiple folders?

SvenMeyer commented 6 years ago

@adrianmcli

adrianmcli commented 6 years ago

@SvenMeyer Yeah I think it's a great idea to output the build artifacts to a user-specified folder. But I want to maintain the structure of a typical truffle project. Having the ability to output build artifacts to multiple paths is also beneficial because there are sometimes multiple frontends/dapps that share the same contracts.

I'm actually a member of the Truffle team myself, and I talked to one of the people on our team today. Version 5 does not solve any of these problems at the moment, and we know that the current contracts_build_directory option is broken.

As for the symlink, I think this works for windows:

mklink \D ..\..\build\contracts contracts

In my PR for the new Truffle React box, I've used it with run-script-us like this:

"link-contracts": "run-script-os",
"link-contracts:linux:darwin": "cd src && ln -s ../../build/contracts contracts",
"link-contracts:win32": "cd src && mklink \\D ..\\..\\build\\contracts contracts"

Of course, you need to install npm install --save-dev run-script-os first. If you have a Windows machine, it would be great if you could confirm whether or not mklink \D ..\..\build\contracts contracts works.

adrianmcli commented 6 years ago

@SvenMeyer Just some follow up comments:

truffle v5.0 beta was just released a few days ago

We're working on a new approach to migrations, so the issues with contracts_build_directory being broken should be fixed once that is done (but I don't know when, I'm not in charge of that).

the symlink solution is not so nice...

Yeah I agree, but it's the best option we have right now unless we use contracts_build_directory which is currently broken and it doesn't sit well with me to moves files around in order to accommodate a broken API. It's much easier/simpler to tell the user to run npm run link-contracts and magically have the right build artifacts accessible.

I am not familiar enough with that topic to judge if ejecting the React app is a good solution

The reason why Create-React-App disallows importing from out of src is because Babel will not compile anything outside of src. There's a lengthy discussion on this here: https://github.com/facebook/create-react-app/issues/834

Since we're just importing a JSON file, it shouldn't be a big deal. But ejecting a CRA project means you get a huge mess of files and folders that will confuse beginners and I am very much against that. The only option here is to use the CRA script to bootstrap a custom project where the plugin is not configured. But that means maintaining our own CRA custom project and I don't want to do that either.

I hope that gives you some more context, but that is the reason why I am more in favour of a seamless NPM script rather than requiring the user to move things around.

SvenMeyer commented 6 years ago

thanks for the extensive follow-up and explanation. As you are a truffle team member, I am sure (hope) you will come up with a nice solution, but meanwhile the symlink is ok, especially with the script and explanation provided (is it on the truffle and github page already?) "unfortunately" I got rid of all Windoze machines, so can't test it.

adrianmcli commented 6 years ago

@SvenMeyer

is it on the truffle and github page already?

I'm still waiting for a teammate to test it on Windows. But this is on my list of things to do for this week.