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

This is not code review. #1

Open
5 tasks
djalilhebal opened this issue Nov 17, 2023 · 0 comments
Open
5 tasks

This is not code review. #1

djalilhebal opened this issue Nov 17, 2023 · 0 comments

Comments

@djalilhebal
Copy link

A few months (or years?) ago, I bookmarked this project since I found it interesting.
Today, I had the chance to test it and check its code.

It's a cool project! :D
I really like the simple/straightforward syntax and API.

I have a few "remarks" though:

  • You are using lodash and wondered if you could get rid of it and use newer JS APIs.
    Yeah, most of them can be replaced with ES6. The code already uses newer APIs (like Object.assign) but sometimes uses Lodash (_.assign) :p
    Anyways, consider using lodash-es instead. It is treeshakeable, meaning the build will be a bit smaller.

  • In the API docs, the "Emitting state for debug purposes" section talks about receiving the state as JSON,
    but the example code uses game.stateListener, which returns an object representation of the game state.

  • The state emitter for stateListener uses a shallow copy (using the spread operator).
    This can be confusing for the programmer if they expect a snapshot of the state.
    The game engine actually mutates the game state, for example this.state.graph.nodeHistory.unshift(...).
    The user may accidentally mutate the data.
    Consider:

    • Using structuredClone (supported in modern browsers and Node, and can be polyfilled).
    • Using Lodash's cloneDeep.
    • Using a silly solution like JSON stringify + parse.
    • Mentioning this as a caveat.
  • Refactoring. In Game, the bodies of Actions.RECEIVE_INPUT and Actions.SET_VARIABLES are the exact same.
    Maybe combine them with an "or"(?).

  • Commenting the code.
    There is a comment/question for Game.addObserver: "What's the difference between this and emitState?".
    If I understand the code correctly,

    • emitState is a private method that emits the current (complete) state to a stateListener and a jsonStateListener (if they exist) whenever the state changes (more like "after handling dispatch calls").
    • while addObserver is a public method to register multiple listeners. They are called whenever a property they are interested in is "changed" (they will be notified if prop X is 10 but we reassign it to 10 using RECEIVE_INPUT/receiveInput).
  • Feature request(?): It would be awesome to have onPassageStart (or Play) and onPassageEnd (or Completed) methods.
    They can be used for animations and stuff.

That is all. Again, cool project!

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

No branches or pull requests

1 participant