-
Notifications
You must be signed in to change notification settings - Fork 196
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
feat: restore typesVersions #2878
Conversation
|
confirmed this works in templates by
pnpm create mud@latest test-project-types
cd test-project-types
pnpm mud set-version --tag main && pnpm install
pnpm mud set-version --tag restore-typeversions && pnpm install |
"bin": { | ||
"gas-report": "./dist/gas-report.js" | ||
}, | ||
"files": [ | ||
"dist", | ||
"out", | ||
"src", | ||
"foundry.toml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you re-added this in a previous PR right? There are several other packages where this was removed as well, so just wanted to make sure that was intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I had added one foundry.toml
in #2867 but can't recall if this was motivated by an error that appeared or if I just spotted it was missing and inconsistent with the rest.
but I don't think these are actually needed (they're similar to tsconfig.json in that they just get used to build)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be certain, I just made a fresh template with this version/PR (published to npm via snapshot action) and confirmed that a local anvil deploy rolled out fine! so I think the resulting app has all it needs inside the out
dir and doesn't need foundry.toml
in the npm package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! I know I wanted to remove it originally, just wanted to make sure there wasn't some weird reason it needed to be kept.
closes #2871
for backwards compatibility with
moduleResolution: "node"
we had removed this in #2828 but it broke projects when upgrading MUD verisons
(intentionally didn't include a changeset because this can be considered part of the changes in #2828 and #2873)