search

clockworklabs / SpacetimeDB

Multiplayer at the speed of light
https://spacetimedb.com
Other
4.41k stars 110 forks source link

Preserve reducer order in schema conversion #1987

Closed RReverser closed 1 week ago

RReverser commented 1 week ago

Description of Changes

This was found while working on #1965. The tests will come in that other PR, but it was suggested that we should land the correctness fix itself sooner.

API and ABI breaking changes

If this is an API or ABI breaking change, please apply the corresponding GitHub label.

Expected complexity level and risk

How complicated do you think these changes are? Grade on a scale from 1 to 5, where 1 is a trivial change, and 5 is a deep-reaching and complex change.

This complexity rating applies not only to the complexity apparent in the diff, but also to its interactions with existing and future code.

If you answered more than a 2, explain what is complex about the PR, and what other components it interacts with in potentially concerning ways.

Testing

Describe any testing you've done, and any testing you'd like your reviewers to do, so that you're confident that all the changes work as expected!

RReverser commented 1 week ago
  • Require the user to sort their reducers when computing IDs

Do you mean with validation check? That would also be fine by me as an alternative.

kazimuth commented 1 week ago

Yeah, I think that's less confusing overall. I can do another branch that adds that check in schema.

RReverser commented 1 week ago

If you think that's better, sure, go for it.

Personally I don't care too much. I do think that preserving whatever order happens to be, is easier for module binding libraries, particularly because a validation error would be visible a bit late in the pipeline (user would get an error only when an example module with mismatched order is compiled & published), but given that we only have 2 such libraries and not creating new one every month or so, it shouldn't be a big deal.

kazimuth commented 1 week ago

Ah, no, you're right. I was confused -- it really doesn't matter inside the module, the module never uses reducer IDs. It's just an interface somewhere else. So, I'm happy with this.

RReverser commented 1 week ago

That's... some new test failure.

This looks good, I think will need a corresponding Private PR.

Also, does this still stand / do I need to change private too?

kazimuth commented 1 week ago

It looks like both test failures here were caused by unrelated heisenbugs on master, we don't need a Private fix. This one might also go away on rerun.