Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: use NPM packages instead of git submodules #1

Merged
merged 3 commits into from
Oct 27, 2023

Conversation

andreivladbrg
Copy link
Member

@andreivladbrg andreivladbrg commented Oct 26, 2023

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

Copy link
Member

@PaulRBerg PaulRBerg left a comment

Choose a reason for hiding this comment

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

Thanks, this is great.

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

@andreivladbrg
Copy link
Member Author

andreivladbrg commented Oct 27, 2023

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
Copy link
Member

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.

foundry-rs/forge-std#208 (comment)

@PaulRBerg PaulRBerg merged commit dafe28b into main Oct 27, 2023
3 checks passed
@PaulRBerg PaulRBerg deleted the build/module-to-npm branch October 27, 2023 14:47
@andreivladbrg
Copy link
Member Author

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
Copy link
Member

It shouldn't be a "necessity":

See foundry-rs/forge-std#208 (comment)

@PaulRBerg
Copy link
Member

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
Copy link
Member

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

@andreivladbrg
Copy link
Member Author

andreivladbrg commented Oct 27, 2023

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
Copy link
Member

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

Ah, yes. Remappings for DSTest! Good point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants