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

feat(store): add experimental config resolve helper #1826

Merged
merged 22 commits into from
Oct 27, 2023

Conversation

alvrs
Copy link
Member

@alvrs alvrs commented Oct 25, 2023

  • Aims to solve one of the main pain points of our current config parser, which breaks on user types in some cases
  • Note that this is just a temporary solution, we're still planning to refactor our config parsing from ground up in the medium term (see prototype new MUD config #1668 for context)

CleanShot 2023-10-25 at 19 55 22

@changeset-bot
Copy link

changeset-bot bot commented Oct 25, 2023

🦋 Changeset detected

Latest commit: 5295bb2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 30 packages
Name Type
@latticexyz/common Major
@latticexyz/block-logs-stream Major
@latticexyz/cli Major
@latticexyz/config Major
@latticexyz/dev-tools Major
@latticexyz/protocol-parser Major
@latticexyz/store-indexer Major
@latticexyz/store-sync Major
@latticexyz/store Major
@latticexyz/world-modules Major
@latticexyz/world Major
@latticexyz/react Major
@latticexyz/abi-ts Major
create-mud Major
@latticexyz/ecs-browser Major
@latticexyz/faucet Major
@latticexyz/gas-report Major
@latticexyz/network Major
@latticexyz/noise Major
@latticexyz/phaserx Major
@latticexyz/recs Major
@latticexyz/schema-type Major
@latticexyz/services Major
@latticexyz/solecs Major
solhint-config-mud Major
solhint-plugin-mud Major
@latticexyz/std-client Major
@latticexyz/std-contracts Major
@latticexyz/store-cache Major
@latticexyz/utils Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

mudConfig({
// Seems like we need `as const` here to keep the strong type.
// Note it resolves to the strong `""` type if no namespace is provided.
// TODO: require the entire input config to be `const`
Copy link
Member

Choose a reason for hiding this comment

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

I remember trying to add as const to our default MUD configs (world config, store config) and it broke downstream types in weird ways. Hopefully we can just add this more specific as const to just the namespace without those downstream effects, but yeah, should aim to as const the whole config.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah the arrays for e.g. enums currently complain if the whole config is as const

Copy link
Member

@holic holic Oct 27, 2023

Choose a reason for hiding this comment

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

Do we wanna add as const to the world/store namespaces here?

}

return {
...config,
Copy link
Member

Choose a reason for hiding this comment

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

since this function isn't being called by default as part of mudConfig, we could just not include config here since we'd have a reference to it already being part of the input and only return the resolved config (and then no need to worry about naming of _resolved)

Copy link
Member Author

Choose a reason for hiding this comment

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

agreed, updated

---

Added an experimental `resolveConfig` helper to refine the output type of `mudConfig` and simplify downstream consumption.
Note that it's not recommended to use this helper externally yet, since the format is expected to change soon while we're refactoring the config parsing.
Copy link
Member

Choose a reason for hiding this comment

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

since this is internal/not recommended for external use, we could just not document this? 🙈

Copy link
Member Author

Choose a reason for hiding this comment

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

we need a changeset to trigger a snapshot release, but since we also added mapObject here there will be a snapshot release anyway, so happy to remove this one

holic
holic previously approved these changes Oct 27, 2023
Copy link
Member

@holic holic left a comment

Choose a reason for hiding this comment

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

🙏 thank youuuuu

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