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

[WIP] local config types #1025

Closed
wants to merge 7 commits into from
Closed

[WIP] local config types #1025

wants to merge 7 commits into from

Conversation

dk1a
Copy link
Contributor

@dk1a dk1a commented Jun 13, 2023

I tried to keep a single mudConfig function for local types, but that seems impossible.
Instead, I separate it into defineConfig and expandConfig, which is basically the approach from #750

plugin definition is easier than globals

Local plugins mostly just need an expandConfig function.

Global plugins need both a global version of an expandConfig function, and a module declaration with interface merging.

Globals could be refactored to be more compact and have good templates, but they'll still be twice as complicated here

config definition is harder than globals

const _typedExpandConfig = expandConfig as ExpandConfig<typeof config>;
type ExpandedConfig = MergeReturnType<typeof _typedExpandConfig<typeof config>>;
export default expandConfig(config) as ExpandedConfig;

Plugin dependencies are harder than globals

It comes down to who has the responsibility for the correct order of plugin dependencies:

  • For global plugins it's the plugin author, because importing plugin deps needs to happen only once and further imports are ignored
  • For local plugins it's the config user, because they have to put all the plugins in the correct order into plugins config option

Scripts are easy

Currently tablegen doesn't even change between global and local plguins.

If expandConfig is moved out of mud.config.mts then tablegen would need to wrap loadConfig with expandConfig, but that's it

Hooks are easy

Similar to https://github.com/latticexyz/mud/pull/1007/files so I don't focus on them here much.

If we want local types, this PR might be best done without hooks first, and then we add them separately

@dk1a dk1a force-pushed the dk1a/local-mudconfig branch from fbe8d5d to a3ef8ce Compare June 15, 2023 13:04
@changeset-bot
Copy link

changeset-bot bot commented Jun 15, 2023

⚠️ No Changeset found

Latest commit: e0ef378

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

@alvrs
Copy link
Member

alvrs commented Jun 16, 2023

thanks a lot for this exploration and detailed thoughts!

config definition is harder than globals

const _typedExpandConfig = expandConfig as ExpandConfig<typeof config>;
type ExpandedConfig = MergeReturnType<typeof _typedExpandConfig<typeof config>>;
export default expandConfig(config) as ExpandedConfig;

What if we create a .mud folder in template projects where we can put a extendedConfig.ts with that content? That way it lives in userland but at the same time the complexity is hidden, it's made clear that this should not be edited manually, and we could even have a mud up cli command that both updates the external mud versions and also updates the contents of .mud. (.mud could basically be like another package with its version aligned with the other mud packages, but not installed via npm but rather via mud up). In that .mud directory we could also potentially put other setup files that have to live in userland (thinking of stuff like setup())

Plugin dependencies are harder than globals
It comes down to who has the responsibility for the correct order of plugin dependencies:

For global plugins it's the plugin author, because importing plugin deps needs to happen only once and further imports are ignored
For local plugins it's the config user, because they have to put all the plugins in the correct order into plugins config option

After our recent discussion about hooks I actually think most dependencies between plugins should be based on hooks. If plugin A expects to run after plugin B, it should hook into plugin A's onAfter hook instead of implicit dependencies based on execution order. The only thing where order would matter then would be if a plugin changes the config by deleting a config type, but I don't think that should be endorsed anyway (but rather only adding stuff to the config)

@dk1a
Copy link
Contributor Author

dk1a commented Jun 19, 2023

What if we create a .mud folder in template projects where we can put a extendedConfig.ts with that content? That way it lives in userland but at the same time the complexity is hidden, it's made clear that this should not be edited manually, and we could even have a mud up cli command that both updates the external mud versions and also updates the contents of .mud. (.mud could basically be like another package with its version aligned with the other mud packages, but not installed via npm but rather via mud up). In that .mud directory we could also potentially put other setup files that have to live in userland (thinking of stuff like setup())

This makes config imports roundabout/obfuscated, but most people wouldn't need to import the config and it should be fine. It's certainly better than doing it in mud.config.ts, or setup.ts (which we want to mostly remove I assume)

After our recent discussion about hooks I actually think most dependencies between plugins should be based on hooks. If plugin A expects to run after plugin B, it should hook into plugin A's onAfter hook instead of implicit dependencies based on execution order. The only thing where order would matter then would be if a plugin changes the config by deleting a config type, but I don't think that should be endorsed anyway (but rather only adding stuff to the config)

Hooks seem like a good solution. Resolving multiple parallel dependencies with hooks would be a pain, but I also can't think of an example here

Before we commit to this, have you considered global hooks again, and do you still believe that locals are better?

@alvrs
Copy link
Member

alvrs commented Jun 20, 2023

Before we commit to this, have you considered global hooks again, and do you still believe that locals are better?

I still think local types are easier to understand for third party developers working on plugins, which I think is the most important factor. Building the foundation for local types on our end might be slightly more complex (lots of typescript black magic) but based on this exploration it seems feasible!

@dk1a
Copy link
Contributor Author

dk1a commented Jun 23, 2023

replaced by #1077
(this PR is a messy prototype, easier to start from scratch and copy some things over)

@dk1a dk1a closed this Jun 23, 2023
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