sablier-labs / sablier-v2-integration-template

Sablier V2 integration template
https://docs.sablier.com
GNU General Public License v3.0
14 stars 2 forks source link

build: use NPM packages instead of git submodules #1

Closed andreivladbrg closed 1 year ago

andreivladbrg commented 1 year ago

Switches from installing v2-core as a git module to installing it as a NPM package.

andreivladbrg commented 1 year ago

We can also install forge-std as a Node.js package from GitHub.

I've asked about this here

My personal opinion is that we could leave the forge-std to be the only dependency installed as a git module because it can be considered a "std" library in Foundry projects.

PaulRBerg commented 1 year ago

I wasn't able to use Forge Std as a Node.js package, so I think this is not a question of preference but of necessity. We will keep installing Forge Std from git submodules.

https://github.com/foundry-rs/forge-std/issues/208#issuecomment-1783027182

andreivladbrg commented 1 year ago

wasn't able to use Forge Std as a Node.js package, so I think this is not a question of preference but of necessity. We will keep installing Forge Std from git submodules.

I don't think you have updated the remappings and the import path correctly: https://github.com/sablier-labs/sablier-v2-integration-template/actions/runs/6668115091/job/18122989105

It shouldn't be a "necessity":

@forge-std/=node_modules/@forge-std/"

import { Test } from "@forge-std/src/Test.sol"
PaulRBerg commented 1 year ago

It shouldn't be a "necessity":

See https://github.com/foundry-rs/forge-std/issues/208#issuecomment-1783027182

PaulRBerg commented 1 year ago

I don't think you have updated the remappings and the import path correctly:

I think I did:

https://github.com/sablier-labs/sablier-v2-integration-template/actions/runs/6668745614

PaulRBerg commented 1 year ago

Don't think you looked at the latest CI run, @andreivladbrg.

andreivladbrg commented 1 year ago

Still not a "necessity". This can be fixed by adding this remappings. You can run locally this branch: https://app.warp.dev/block/MtrSbAaMRDnU5ZrsyFFLul

A easier fix would be to import in forge-std from: import {DSTest} from "../lib/ds-test/src/test.sol";

Anyway, I am not in favor in installing forge-std as NPM, as said in my initial comment.

PaulRBerg commented 1 year ago

Still not a "necessity". This can be fixed by adding this remappings

Ah, yes. Remappings for DSTest! Good point.