-
Notifications
You must be signed in to change notification settings - Fork 203
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
fix: codesplit cli code to allow mud.config.ts to be imported in the browser #809
Conversation
Unclear why tests are failing, all are passing locally and there were no changes to the store package that would influence the forge tests. Trying |
maybe time for #746? |
Looks like main is failing too. I think it might be this: foundry-rs/foundry#4920
It's a little unclear by the above "nested in the CALL subtree" description if this should succeed or fail in our case, but I noticed that This feels like a bummer constraint if so, and wonder if there are ways around this or alternative vm helpers. I asked about this in the Foundry Support Telegram group. If there's not a quick/clear way to test for emits in the same way with the latest foundry version, we may need to pin our version for now. |
Another option is to use Vulcan test utils, which has some nicer ergonomics: https://github.com/nomoixyz/vulcan mc.doSomethingElse();
// Check event emissions
expect(address(mc).lastCall()).toHaveEmitted(
"SomeEvent(address,bytes32,uint256)",
[address(1).topic(), any()], // Event topics
abi.encode(123) // Event data
); (I also have an open issue and they have a pending PR for better gas reporting) |
opened an issue for this since its unrelated to this PR: #814 |
@@ -35,5 +35,5 @@ jobs: | |||
uses: ./.github/actions/require-empty-diff | |||
|
|||
- name: Run tests | |||
working-directory: ./examples/${{ matrix.example }} | |||
working-directory: ./templates/${{ matrix.template }} |
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.
nice catch
packages/config/package.json
Outdated
@@ -11,7 +11,8 @@ | |||
"type": "module", | |||
"exports": { | |||
".": "./dist/library/index.js", | |||
"./register": "./dist/register/index.js" | |||
"./register": "./dist/register/index.js", | |||
"./cli": "./dist/cli/index.js" |
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.
should this be /node
rather than /cli
?
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 was torn between the two, but now agree node
fits better - changed it for both config and world exports
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.
Looks good! Only question is if we should call this export node
rather than cli
, if node is the intended environment. cli
sounds too use case specific and very internally oriented, and if that's what we're aiming for, maybe we name it cli-for-internal-use-only
haha
Separately wondering if we should just bite the bullet on #746 (maybe not before the hackathon but quickly following?)
Unfortunately I think it's too close to the hackathon now to refactor this, but agree with shortly after! |
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 like node
We need to be able to import
mud.config.ts
in browser code to infer types for various client libraries (egstore-cache
).This PR fixes a couple issues that prevented this:
mudConfig
also imported code intended for node environments, which causedvite
in consumer projects to error (example:global
(used for mud plugins extending the config) was not defined in browser contexts: