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

Silo: Actions are not carried over into combined silos #182

Closed
ThoughtSpinnr opened this issue Jan 4, 2024 · 2 comments · Fixed by #183
Closed

Silo: Actions are not carried over into combined silos #182

ThoughtSpinnr opened this issue Jan 4, 2024 · 2 comments · Fixed by #183
Assignees
Labels
bug Something isn't working

Comments

@ThoughtSpinnr
Copy link

When combining silos together through Silo.combine(), the actions from the provided silos are not carried over into the combined silo.

This may be the root cause of #97.

Example code:

local ReplicatedStorage = game:GetService("ReplicatedStorage")

local Packages = ReplicatedStorage.Packages
local Shared = ReplicatedStorage.Shared

local Silo = require(Packages.Silo)

local KillsFragmentInfo = require(Shared.Silos.KillsFragment)
local KillsFragment = Silo.new(KillsFragmentInfo.State, KillsFragmentInfo.Modifiers)

local DeathsFragmentInfo = require(Shared.Silos.DeathsFragment)
local DeathsFragment = Silo.new(DeathsFragmentInfo.State, DeathsFragmentInfo.Modifiers)

local CompleteFragment = Silo.combine({
    Kills = KillsFragment,
    Deaths = DeathsFragment,
})

print(KillsFragment)
print(DeathsFragment)
print(CompleteFragment)

Output:

image

@Sleitnick Sleitnick self-assigned this Jan 6, 2024
@Sleitnick
Copy link
Owner

Here's some info about the issue:

  1. You can still dispatch actions through the original silos and it will affect the combined silo.
  2. Populating the combined silo with all the actions has the possibility of name conflicts.

Regarding 1, this is a temporary workaround.

Regarding 2, this makes the solution a bit complicated. While the internal modifier adds a prefix to avoid the naming conflict, the name of the action remains the same. Thus, with possible naming conflicts, either (a) an error should be thrown if a naming conflict occurs, or (b) a prefix should also be added to the action name (which I think would be confusing). I am leaning toward option (a). Just throw an error for naming conflicts.

@Sleitnick
Copy link
Owner

Went with option (a). Actions are combined, and an error is thrown if a name conflict is found. Published in Silo v0.2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants