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(world): SnapSync config plugin #848

Merged
merged 17 commits into from
May 18, 2023
Merged

feat(world): SnapSync config plugin #848

merged 17 commits into from
May 18, 2023

Conversation

Kooshaba
Copy link
Contributor

No description provided.

import { resolveTableId } from "@latticexyz/config";

export default mudConfig({
snapSync: true,
Copy link
Member

Choose a reason for hiding this comment

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

just as a general note, I think this is the first time the mud config has had an option that could effect network code

since we're passing in the mud config to the network config, maybe should just use this key to determine if snap sync should be used?

@@ -1,7 +1,8 @@
import { mudConfig } from "@latticexyz/world/register";
import { mudConfig } from "@latticexyz/world/snapSync";
Copy link
Member

Choose a reason for hiding this comment

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

this approach looks like it wouldn't allow for composability of plugins

@dk1a is this how we're expected to create/use plugins?

Copy link
Member

Choose a reason for hiding this comment

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

this should work well, and it's the approach I use for world the plugin. It just means this plugin can't be used without store and world, and implicitly registers those so you can skip em (or you can still import em).
Here I guess it just looks a bit weird/misleading

Copy link
Member

Choose a reason for hiding this comment

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

maybe keep import { mudConfig } from "@latticexyz/world/register"; and have snapSync be 2nd?

Copy link
Member

@dk1a dk1a May 16, 2023

Choose a reason for hiding this comment

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

a more explicit requirement of world being registered could work instead, but we don't really have a good way to check that atm (plugin ids?)
on the other hand you don't need a good way to check if world plugin is used, you can just check that tables and modules exist

packages/world/src/modules/snapsync/ts/typeExtensions.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,6 @@
import "@latticexyz/store/register";
Copy link
Member

Choose a reason for hiding this comment

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

should the plugin code be in src/ts/plugins, or #584?
modules are more like things that can be created/used by plugins

Copy link
Member

Choose a reason for hiding this comment

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

for now probably in src/ts/plugins, and later in its own package (or 584)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there another plugins dir i don't know about or do you just mean one here?

Copy link
Member

Choose a reason for hiding this comment

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

currently there is no plugins dir at all, I just meant to add one in packages/world/src/ts/plugins (temporarily, until we separate it as its own plugin)

@alvrs alvrs merged commit 17446d7 into main May 18, 2023
@alvrs alvrs deleted the kooshaba/snap-sync-plugin branch May 18, 2023 15:36
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.

4 participants