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

Format Solidity + JS + TS files, pnpm workspace #97

Merged
merged 16 commits into from
Apr 12, 2024
Merged

Conversation

bpierre
Copy link
Contributor

@bpierre bpierre commented Apr 4, 2024

The format script is to be ran before merging this PR, whenever we are ready.

@bpierre bpierre closed this Apr 4, 2024
@bpierre bpierre reopened this Apr 4, 2024
@danielattilasimon
Copy link
Collaborator

Could we move the formatting into the root of the repo? I know this requires one more pnpm i, but it would let us have a single dprint config for every project in the repo (of which we will inevitably have more than just the contracts).

Also, the dprint plugin for VS Code only works if the config file is in the root directory.

@bpierre
Copy link
Contributor Author

bpierre commented Apr 5, 2024

Good point yes! I am wondering: would it make sense to move to a monorepo now that we have moved to pnpm?

From my experience doing this with pnpm after yarn:

  • Doing pnpm i from the root or any of the package does the same thing and installs all the workspaces dependencies (no specific instruction for contributors other than using pnpm).
  • Generally, I found pnpm much easier to work with than Yarn 2 & 3 (haven’t tried 4) for monorepos.

I think the only change needed would be to add a package.json and a pnpm-workspace.yaml file at the root level.

@danielattilasimon
Copy link
Collaborator

It occured to me too. We should give it a try with pnpm workspaces. The reason I swore off of using a monorepo again was mostly because of Yarn and the nightmare that is package hoisting. Thankfully, pnpm does none of that crap.

@bpierre bpierre changed the title Add a script to format Solidity + JS + TS files pnpm workspace + format Solidity + JS + TS files Apr 9, 2024
@bpierre
Copy link
Contributor Author

bpierre commented Apr 9, 2024

It occured to me too. We should give it a try with pnpm workspaces. The reason I swore off of using a monorepo again was mostly because of Yarn and the nightmare that is package hoisting. Thankfully, pnpm does none of that crap.

I added the change to the PR 🔥

@bpierre bpierre marked this pull request as ready for review April 9, 2024 09:37
@bpierre bpierre changed the title pnpm workspace + format Solidity + JS + TS files Format Solidity + JS + TS files, pnpm workspace Apr 9, 2024
Now that we have a workspace in the root of the repo, we also have
node_modules there.
dprint.json Outdated Show resolved Hide resolved
"json": {
},
"markdown": {
"arrowFunction.useParentheses": "force"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also I thought this one would be nice to have for consistency. I also removed useBraces: "always" compared to what we had on frontend/, to be a bit less strict. Also no strong opinion about these two, what do you think @danielattilasimon?

@danielattilasimon danielattilasimon merged commit e000686 into main Apr 12, 2024
6 checks passed
@bpierre bpierre deleted the format branch April 12, 2024 10:41
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