juanfranblanco / vscode-solidity

Visual Studio Code language support extension for Solidity smart contracts in Ethereum https://marketplace.visualstudio.com/items?itemName=JuanBlanco.solidity
MIT License
892 stars 192 forks source link

Feature request: allow multiple paths for `packageDefaultDependenciesDirectory` #149

Open PaulRBerg opened 5 years ago

PaulRBerg commented 5 years ago

I can currently set the location of "node_modules" like this:

# user settings
{
  ...
  "solidity.packageDefaultDependenciesDirectory": "node_modules"
  ...
}

The structure of my repo looks like this:

Since I'm using yarn workspaces, most of my third-party contracts are cached at the root. vscode-solidity has no trouble with those contracts.

However, sometimes I have to use the nohoist feature, which moves the contracts down below in "package1", "package2", etc. When that happens, I get this error:

Source "@openzeppelin/contracts-ethereum-package/contracts/math/SafeMath.sol" not found; File import callback not supported

This could be solved by allowing multiple (glob) paths in the packageDefaultDependenciesDirectory user setting. Example:

# user settings
{
  ...
  "solidity.packageDefaultDependenciesDirectories": [
    "node_modules",
    "packages/package1/node_modules",
    "packages/package2/node_modules"
  ]
  ...
}
juanfranblanco commented 5 years ago

Hi @PaulRBerg,

Obviously order of preference is important.. and you could end up using dappsys libraries with zeppelin. I will do some experimentation and see how it goes.

PaulRBerg commented 5 years ago

Thank you @juanfranblanco 😊

ottodevs commented 5 years ago

Hi @juanfranblanco , just my thoughts on this: Truffle resolves the import dependencies as it should be, how hard is to copy the module resolution mechanism from there? This issue is apparently recurrent in time (#31) the solution to add a packageDefaultDependenciesDirectory just solves the problem partially.

In my opinion a consistent solution should be using the same resolution system that truffle or any other linter uses to resolve the imports in the target file

Or as @PaulRBerg proposes, an array of paths, but that will still be too much constrained to specific projects...

Another solution would be to allow using globs in the path, that would be a more generic and valid solution as well.

The main issue is that nowadays mono-repo project structures are quite common, and those repos normally have a set of dependencies installed directly into the root node_modules and other set of deps eventually installed into local node_modules subfolders for nested packages. Truffle is handling this situation correctly, so the key should be there.

I tried to struggle and change some code in the extension myself, should it be modified into initialiseProject function from projectService.ts ? I would like to help with this but I am not very familiar with this project structure and code.

juanfranblanco commented 5 years ago

The paths need to support also the DappHub / Dappsys structure as used by Maker, so the multiple path array could solve the problem. https://github.com/dapphub

juanfranblanco commented 5 years ago

Although as we have seen in DappSys, we have a path + contractFolder, hence there are two settings. So it is going to need an array of these objects containing both values.

Other thought is that imports will need to be resolved before hand, ie do they exists before rewriting them.

Many years ago Christian (solidity) and I discuss the pattern. Which mainly for Libraries the import does not start with any ".", this indicates that these need to be resolved using the library mechanism.

deluca-mike commented 3 years ago

Is this resolved yet? My imports are all flagging as not found, despite existing and compiling, resulting not linting or other error detection (because this error is a show stopper for processing the rest of the file).

cameel commented 3 years ago

Truffle resolves the import dependencies as it should be, how hard is to copy the module resolution mechanism from there?

Truffle's path resolver is actually pretty modular and published as a separate npm package:

Maybe it would be possible to just use it as a dependency?

I think it might even accomodate the dappsys use case. For example handling of node_modules is just one of several specializations of ResolverSource. It should be possible to write one for handling the lib/ directory too.

juanfranblanco commented 3 years ago

@cameel It will be much easier to extend it to use array paths for dependencies than adding a dependency to truffle. As usual making sure that it does not break current deployments.

juanfranblanco commented 3 years ago

Edit: And yes I have not got around to do it, as most people just use node_modules as the main dependency folder.

cameel commented 3 years ago

Yeah, agreed on that, a path list is definitely simpler and easier to maintain than a full-blown resolver. The downside though is that with some less common project layouts (like node_modules not at the root or not using node_modules at all) some people seem confused about how to configure the extension. Using a ready-made resolver would help with that, but of course at the additional cost of having to manage the dependency. Looks like you're thinking about introducing a project file instead (https://github.com/juanfranblanco/vscode-solidity/issues/51#issuecomment-851175360), preferably in a format shared with other tools, and I think that would be a great solution too.

Incidentally, we'll soon be adding a similar feature in the Solidity compiler and we went for a path list solution too: https://github.com/ethereum/solidity/issues/11409. In our case it's motivated by not wanting to make too many assumptions about how projects are structured so there won't even be any defaults other than the working directory. The compiler itself should be agnostic about that.

BTW, for anyone wondering about this, the compiler feature is only for command-line usage with an import callback so unfortunately it will not be of any help here - this extension uses Standard JSON input without a callback.

most people just use node_modules as the main dependency folder.

Just wanted to add some nuance here since I've been digging into this exact topic recently :) That's generally true but only because the ecosystem is close to a monoculture now in that regard. People use almost exclusively JS-based tools (i.e Truffle or Hardhat) but there are also some less popular tools that do not rely on npm and thus do not use node_modules. For example Brownie installs packages globally, in ~/.brownie/packages while dapp-tools relies on symlinks within project root. Even with Truffle a single node_modules located at the project root is not universal. You can have a global node_modules or even use both. You can also install packages from EthPM or even import interfaces generated automatically from JSON files with abi-to-sol. node_modules/ is a reasonable default right now but I hope that it does not start being treated as a universal standard. That would be a significant hurdle for non-JS frameworks.

kay-is commented 2 years ago

I played around with Solidity and did some Hardhat example projects, effectively ending up with a mono-repo.

I got the zeppelin import error, but the worst part was that the error didn't help much. It just told me that it couldn't find the import. Searching online didn't lead here.

I tried multiple configurations for the contract/node_modules directory but couldn't get it to work.

In a chat, someone mentioned that "it's sad the vscode solidity plugin doesn't work with mono-repos" when I finally found my problem.

It would be cool if mono-repos would work directly, or at least the errors would be a bit more insightful.

Anyway, excellent extension, thanks for it!!

juanfranblanco commented 2 years ago

hi @kay-is and all,

You can use "remappings" now that will allow you to support multiple directories and specific mappings https://github.com/juanfranblanco/vscode-solidity#remappings

Please let me know how it goes.

Edit: Also as you have mono-repos, now vscode supports having multiple "virtual" folders in a workspace, each one will allow you to add any folder in a workspace, and each folder can have their own "remapping.txt" file.

juanfranblanco commented 2 years ago

@kay-is if this does not work.. let me know!

kay-is commented 2 years ago

@juanfranblanco when I have a structure like that:

workspaceRoot/greeter/contracts workspaceRoot/greeter/node_modules workspaceRoot/fundraiser/contracts workspaceRoot/fundraiser/node_modules

How would I define the correct mappings?

juanfranblanco commented 2 years ago

Ah, in that scenario the best thing will be to create (at the moment) a workspace and add each folder to the workspace. Each folder (greeter, fundraiser) having their own "remappings.txt" file.

Although as each folder is just using "node_modules" by adding each folder to the workspace, each folder will be considered a root, so will be using "node_modules" as a folder without having to add a "remappings.txt" file.

Now, in the future if, we have a project file as a standard, where this file is located that could be considered the root, so I was thinking of using the "remappings.txt" as such.

kerloom commented 2 years ago

I think this might be a good idea not only for mono-repos. How would one go about and set up the dependencies folders for globally installed packages? In my case I have truffle installed globally while other packages like openzeppelin locally.

juanfranblanco commented 2 years ago

@kerloom for this you can use remappings, Edit: this is what it is used by brownie Edit2: You can put your remappings in the settings also.

juanfranblanco commented 2 years ago

@kerloom there is also now specific remapping settings for unix / windows so these can be shared globally in a specific location, working directory

carllippert commented 2 years ago

@juanfranblanco how would you do a project structured like this with yarn workspaces:

I am using remappings.txt inside the foundry file. Have .vscode settings inside foundry set to lib vs node_modules for foundry

Still getting "File import callback not supported"

Perhaps I'm just doing it the hard way and need a different structure.

Thanks!

juanfranblanco commented 2 years ago

@carllippert the remappints.txt needs to be set in the root folder. Let me know how it goes.