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

Fix/not linking truffle projects #1096

Merged
merged 13 commits into from
Jan 24, 2019
Merged

Conversation

honestbonsai
Copy link
Contributor

@honestbonsai honestbonsai commented Jan 23, 2019

Fixes #1078

This PR does the following:

  1. Fixes contracts_build_directory needing to be a direct child of build_directory (which defaults to ./build) to get a Truffle project to link.
  2. Allows you to put your contracts_build_directory as deep as you want as a descendant of your Truffle project. (e.g. path/to/project/a/b/c/d/contracts_build_directory)
  3. Allows you to put your contracts_build_directory outside of your Truffle project (i.e. not a descendant).

It does not do:

  1. Handle the renaming of a contract in the directory. This seems like a current bug with Ganache. Currently in develop and this branch, if you rename a contract's file name, all other contracts in Ganache disappear. If you try to rename it back to the original name, nothing changes. I considered this out of scope, will open a new issue for this.
  2. Subfolders of contract artifacts within contracts_build_directory. Didn't seem like it was in scope for this card. If enough people want it, then a new issue/feature request should be made.

FYI:

  1. Does seem to be slightly slower than the current implementation, since I'm recursing through the chain of directories. My feeling is that most people will just have their contracts_build_directory inside their project anyways, so this shouldn't be too big of an issue, but if you think it is let me know.

@@ -89,18 +99,19 @@ class ProjectFsWatcher extends EventEmitter {
) {
this.startWatchingContracts();
}

if (tail[0] && filename === path.basename(tail[0]))
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this and lines 97/98 (won't let met comment there) should not be using the basename function, and be more something like

path.join(head, filename) === path.normalize(this.project.config.contracts_build_directory)

I don't think it's an issue either way as startWatchingContracts() will check to see if the directory exists before watching. Food for thought

Copy link
Contributor

@mikeseese mikeseese left a comment

Choose a reason for hiding this comment

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

If tests pass, and you verified this solves the issue, and you have verified that it works with default settings still (it should), this LGTM!

My other comment is for you to decide if you want to change it or not. Either way, has my approval.

@honestbonsai
Copy link
Contributor Author

honestbonsai commented Jan 24, 2019

Yup, tested with truffle unbox drizzle and it works out of the box (haha) and also default empty truffle-config.js.

@honestbonsai honestbonsai merged commit 8beeb4f into develop Jan 24, 2019
@honestbonsai honestbonsai deleted the fix/not-linking-truffle-projects branch January 24, 2019 18:54
@0xjjpa
Copy link

0xjjpa commented Jan 25, 2019

Just wanted to comment that I've tested this with truffle unbox react and it works as well. Finishing a course w/Solidity this weekend with it, talking about good timing!

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.

3 participants