boardgame.io icon indicating copy to clipboard operation
boardgame.io copied to clipboard

feat: Change move and hook signature

Open delucis opened this issue 4 years ago • 3 comments

📣 Try the pre-release today 📣

You can start working with the new hook signature by installing the alpha release channel from npm:

npm i boardgame.io@alpha

If you run into any issues, let us know!


Closes #662.

This PR implements the change from (G, ctx) to ({ G, ctx, playerID, ...plugins }).

Game code that currently looks like this:

const move = (G, ctx, arg) => {
  const score = arg * ctx.random.D6();
  G.players[ctx.playerID] = score;
  if (score < 12) ctx.events.endTurn();
};

Would become:

const move = ({ G, random, playerID, events }, arg) => {
  const score = arg * random.D6();
  G.players[playerID] = score;
  if (score < 12) events.endTurn();
};

This applies across moves and hooks.

To-do

The following parts of game code haven’t been changed yet:

  1. [x] playerView is currently still (G, ctx, playerID) => G. Should this become ({ G, ctx, playerID }) => G?

  2. I haven’t updated the bot interface at this point. Should the following be changed for consistency?

    • enumerate(G: any, ctx: Ctx, playerID: PlayerID)

    • Various other methods in the MCTS bot implementation

  3. [x] The undoable method of the long-form move syntax, which can be (G, ctx) => boolean. Should this become ({ G, ctx }) => boolean?

Notes

  • In refactoring tests, I noticed that this change often results in (to my mind) clearer game code and will hopefully help to clear up ongoing confusion about ctx being different in moves and on the client. Types are also greatly improved.

  • One thing this helped clarify for me, was that playerID (currently ctx.playerID) is only available in moves, not in any hooks. Given the slightly confusing nature of playerID in hooks, this may not be a bad thing. For example, it’s not necessarily easy to understand why playerID in turn.onBegin is the ID of the player whose action caused the previous turn to end, not the ID of the player whose turn is beginning. That said, if we add onEnd/onBegin events for stages as proposed in #608, there will be a need to revisit this in the future.

  • While updating the docs, I noticed it wasn’t very clear what to call the whole object that contains G, ctx etc. I think of this whole lot as “context” for lack of a better word, but that would be confusing as ctx is already in use. I wonder if in fact, this refactor might be an opportunity to rename ctx itself to something more descriptive. Not sure what that name would be, but flow comes to mind.

    (context: { G: any; flow: Flow; ... }) => {
      const { G, flow } = context;
      console.log(flow.turn, flow.currentPlayer);
      if (!flow.activePlayers) { /* ... */ }
    }
    
  • I’m not sure when will be best to merge and release this as obviously it’s a significant breaking change for everyone, but I wanted to get the ball rolling and we can keep this PR up-to-date as other changes arrive on the main branch.

Migration Paths

This change is pretty impactful across game definitions. Here are some migration strategies.

1. Rewrite your functions

Ideally, you would rewrite your game to use the new syntax. In general this will result in simpler code. For example:

// Destructure G, ctx, and plugin APIs from the first argument
- function move(G, ctx, arg) {
+ function move({ G, random, playerID }, arg) {

// Access plugin APIs directly instead of from ctx
-   G.diceRoll = ctx.random.D6();
+   G.diceRoll = random.D6();

// Access playerID directly instead of from ctx
-   G.players[ctx.playerID].score += G.diceRoll;
+   G.players[playerID].score += G.diceRoll;

}

2. Use a stop-gap plugin for compatibility

If you don’t want to rewrite your moves, you can use a custom plugin to wrap those functions so they continue to receive the old style of arguments.

const CompatibilityPlugin = {
  name: 'compat-plugin',

  fnWrap: (fn) => ({ G, ctx, ...api }, ...args) => {
    const ctxWithAPI = { ...ctx, ...api };
    return fn(G, ctxWithAPI, ...args);
  },
};

const game = {
  plugins: [CompatibilityPlugin],

  moves: {
    oldStyle: (G, ctx) => {
      // Use ctx.random etc. as before
    },
  },
};

3. Gradual migration

Using a plugin is an all-or-nothing approach — it forces you to continue using the old-style of arguments. If you want to gradually migrate your code, you’ll need to wrap only functions you haven’t updated yet. To do this we could adapt the fnWrap method from above to selectively wrap certain moves.

// fnWrap adapted to account for long-form move objects, which
// boardgame.io does automatically when applying plugin wrappers
const moveAdapter = (moveFnOrObj) => {
  const isLongFormMove = typeof moveFnOrObj.move === 'function';
  const fn = isLongFormMove ? moveFnOrObj.move : moveFnOrObj;
  const move = ({ G, ctx, ...api }, ...args) => {
    const ctxWithAPI = { ...ctx, ...api };
    return fn(G, ctxWithAPI, ...args);
  };
  return isLongFormMove ? { ...moveFnOrObj, move } : move;
};

const game = {
  moves: {
    oldStyle: moveAdapter((G, ctx, arg) => {
      // Use ctx.random etc. as before
    }),
    newStyle: ({ G, ctx }, arg) => {},
  },
};

Depending on how your project is structured, you may be able to simplify applying this adapter rather than doing it manually for each function, for example if you store your moves something like this:

const moves = { rollDice, moveToken, capture, build };

You could apply the adapter programmatically:

for (const key in moves) {
  moves[key] = moveAdapter(moves[key]);
}

Then you could migrate by splitting your moves into two collections:

const oldstyleMoves = { rollDice, build };
const migratedMoves = { moveToken, capture };

for (const key in oldstyleMoves) {
  oldstyleMoves[key] = moveAdapter(oldstyleMoves[key]);
}

const moves = { ...oldstyleMoves, ...migratedMoves };

General Tips

Use Typescript to help find errors

If you’re already using TypeScript, make sure you use boardgame.io’s bundled types for your game to catch changes:

import type { Game, Move } from 'boardgame.io';

interface G {
  score: number;
}

const game: Game<G> = { /* ... */ };
const rollDice: Move<G> = ({ G, random }) => { /* ... */ };

If you’re writing plain JavaScript, you can often still use TypeScript types to get some in-editor hints by using Typescript’s JSDoc syntax (VS Code does this well for example).

/**
 * @typedef {{ score: number }} G
 */

/**
 * @type {import('boardgame.io').Game<G>}
 */
const game = { /* ... */ };

/**
 * @type {import('boardgame.io').Move<G>}
 */
const rollDice = ({ G, random }) => { /* ... */ };

delucis avatar Jan 17 '21 22:01 delucis

Is this change still proposed or overcome by events? Context.ctx seems confusing to me but flow somehow less appealing. The stage hooks might afford better outcomes in state modeling which is my real interest -- this is now a year old PR and might be too stale to consider but like that you are considering evolving the state model to help with clarity -- it was the biggest lift in trying to understand bgio when I took first steps.

cwatsonc avatar Nov 18 '21 08:11 cwatsonc

@cwatsonc This branch is up to date and I do intend to merge it. Potentially soon, as I quite like the idea of this bigger change landing in v0.50, which is our next release.

Just to emphasise: there are no functionality changes or features involved here, it’s just a question of how arguments are provided to game functions, i.e. (G, ctx) => {} vs a new {{ G, ctx }) => {}.

delucis avatar Nov 18 '21 09:11 delucis

Will this be merged? This is a huge improvement over the current state of things.

YoavHortman avatar Sep 14 '22 14:09 YoavHortman

I received feedback that people really want this and are already using the alpha. I vote for finishing reviewing/merging all other outstanding PRs and doing a minor release. After that, we merge this PR and do a major release. What do you think, @delucis ?

vdfdev avatar Oct 05 '22 15:10 vdfdev

Major would mean [email protected]. Not totally opposed but I think we could also release this as a minor (0.50.0 was my target for merging this) — minors below v1 are treated as major semver by npm, so it’s OK to introduce breaking changes from minor to minor.

So if we have stuff we want to get out before this, that could be 0.50.0 and then this as 0.51.0 or 1.0.0 if we’re ready! (One thing I had always thought needed work prior to v1 was the bot/AI architecture, but improving that can be a v2 goal too if we want).

delucis avatar Oct 06 '22 10:10 delucis

Oh ok, I don't mind either way, as long as we do a release before this PR goes in, because it might take a while to redactor all games to it. Tonight I will try to review your code, and then we can proceed!

vdfdev avatar Oct 06 '22 16:10 vdfdev

Sounds good. Thanks @vdfdev! 🙌

delucis avatar Oct 06 '22 21:10 delucis

Uh oh, I think I borked the merge, will revert and fix it.

vdfdev avatar Oct 11 '22 04:10 vdfdev

Seems like https://github.com/boardgameio/boardgame.io/commit/aa99a9cce28012cb747fa6db8b3f8ad73c28be0a conflicted with this PR. I was able to fix one test, but there is one that still fails. I will keep trying to fix it

vdfdev avatar Oct 11 '22 05:10 vdfdev

@delucis Hey, I have a pretty huge bgio codebase and after the update got over 500 typescript errors. I wanted to use the stop-gap-plugin described above, but as far as I can tell this will only cover the issue at runtime. Typescript will still complain about incorrect signatures. Is there a way to fix this or am I stuck with either fully migrating or staying with a version <50?

on3iro avatar Feb 18 '23 08:02 on3iro