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

Roadmap: chess.js 1.0.0 #319

Open
35 of 44 tasks
jhlywa opened this issue Apr 1, 2022 · 26 comments
Open
35 of 44 tasks

Roadmap: chess.js 1.0.0 #319

jhlywa opened this issue Apr 1, 2022 · 26 comments

Comments

@jhlywa
Copy link
Owner

jhlywa commented Apr 1, 2022

After 13 years of pre-1.0.0 development .... it's time.

This is a working document to track planned features/changes and their statuses for version 1.0.0. Please comment if there's something you'd like to see included or changed. I appreciate and value your feedback. Check back frequently for updates.

I'll push a basic typescript implementation to dev branch to use as starting point.

Please wait for the dev branch before submitting PR's.

dev branch is now active

Proposed 1.0.0 Changes

  • Code

    • Rewrite library in typescript
    • Use camelcase for API and everything else
      • enforce with eslint rule
    • Use exceptions in error states instead of returning null. This will allow the library to provide more detailed error messages for common issues (e.g. bad moves, bad FEN, bad PGN). The functions that throw exceptions are listed in the API Changes section.
      • throw exception for bad FEN in .load()
      • throw exception for bad FEN in Chess() constructor
      • throw exception for bad PGN in loadPgn()
      • throw exception for bad SAN in move()
      • throw exception for bad move object in move()
    • Switch to using the sloppy FEN/PGN parser by default. The phrase sloppy is kind of passive aggressive, so maybe use the terms permissive and strict.
      • load in permissive mode (the default mode) should let the user load FEN without supplying castling rights, ep square, and move numbers (e.g. chess.load(rnbqkbnr/pppppppp/8/8/8/8/PPPPPPPP/RNBQKBNR w))
      • load and loadPgn should allow an option to use the strict parser
    • moves() should work even if there are no kings on the board.
    • Only set the en passant square when the opponent can legally make an en passant move. (see Two square pawn moves generate fen with en passant square even when no pawn can capture. #294)
    • Add a 'lan' (better name or is this ok??) property for long algebraic notation (see generate uci moves for use with engines like stockfish #259)
    • Improve FEN validation. It should detect common hand-entered such as missing kings and incorrect castling rights.
    • Use fast-check for property-based testing
  • Docs

    • create a chess.js logo (maybe this is something the community can vote on??)
    • Move documentation to chessjs.org
    • Add an FAQ section to answer common questions
  • Other

    • Github issue template ensuring the reporter know the rules of chess before filing a bug (en passant gets flagged as a bug a few times a year)
    • Prefer stackoverflow for chess.js usage questions, github issues for bugs

API Changes

  • game_over -> isGameOver
  • in_check -> isCheck
  • in_checkmate -> isCheckmate
  • in_draw -> isDraw
  • is_stalemate -> isStalemate
  • in_threefold_repetition -> isThreefoldRepetition
  • insufficient_material -> isInsufficientMaterial
  • load_pgn -> loadPgn
  • set_comment -> setComment
  • get_comment -> getComment
  • get_comments -> getComments
  • delete_comment -> deleteComment
  • delete_comments -> deleteComments
  • validate_fen -> validateFen
  • The following functions should throw exceptions in the event of an error
    • Chess constructor
    • load
    • loadPgn
    • move

Questions

  • Should acronyms be capitalized in function names (e.g. loadPgn vs loadPGN)? Consider the impact this could have on the .fen() and .pgn() functions. I'm leaning towards no, but it's up for discussion. Decided not to capitalize acronyns.
  • Should we adopt the chess.js TypeScript definitions from DefinitelyTyped as a basis for our types?

Future Changes beyond 1.0.0

  • use piece lists for increased move generation performance
  • use a parser generator to parse PGN
  • add support for RAV (recursive annotation variations)
  • add support for null moves
  • chess960 support

Issues for First Time Contributors

  • add a default separator='\n' parameter to the ascii() function
@coffeeDev98
Copy link

coffeeDev98 commented Apr 4, 2022

Hey @jhlywa ,
Great to see this is back in development!! Have been using the stable package for our project...
Had a request... Is it possible to add variations support...? Or Is it something that you're planning to add...?
Like in @aaronfi 's project chess-es6

@jhlywa
Copy link
Owner Author

jhlywa commented Apr 4, 2022

The variation code will most likely come later in version 2.0.0 (I refer to it above as RAV - Recursive Annotation Variation). The PGN loading code needs to be rewritten from the ground up. It's currently a set of hand crafted rules that are a little tedious to maintain and follow. A better approach would be to include the PGN grammar and then use a parser generator to generate the parsing code. Once this is done, adding support for variations should be significantly easier.

@coffeeDev98
Copy link

Hey @jhlywa,
Had a doubt... How would you go about with creating the PEG file / Grammar file? Will you be creating one from scratch or referring to an existing one like, PGNViewer...? Looking into the PGN Grammar right now..

@DevAndrewGeorge
Copy link
Contributor

Just began using this repo, but this is exciting! I'll happily make some contributions once the FIX_ME branch is live.

@jhlywa
Copy link
Owner Author

jhlywa commented May 13, 2022

@coffeeDev98 I'd be interesting in creating the grammar from scratch. I think it would give us a better understanding of how the PGN parser works, and frankly, I think it would be easier than maintaining the existing regexes.

@jhlywa
Copy link
Owner Author

jhlywa commented May 14, 2022

Hi all, I just pushed the new dev branch this morning. I manually copied over all of the tests from the 0.13.2 tag and added a few extras where necessary. However, there does seems to be a discrepancy in the number of tests cases jest is running in 0.13.2 (339 test cases) and dev (237 cases). We should investigate this.

@coffeeDev98
Copy link

coffeeDev98 commented May 19, 2022

Hey @jhlywa,
I shall do a deep dive of the code before contributing... Any advice on the same...? I'm new to contributions on github... 😅.
Also, would be great if you could share some resources on grammar files. I've gone through the grammar file of PGNViewer but hitting a roadblock as far as other examples are considered

@DevAndrewGeorge
Copy link
Contributor

@jhlywa, did you ever resolve the test issue? I don't see as big of a discrepancy between v013.2 and dev as you state:

Andrews-MacBook-Pro:chess.js andrew$ git checkout v0.13.2; npm test 2>&1 | grep "Tests:"
HEAD is now at 426e7ed 0.13.2
Tests:       338 passed, 338 total
Andrews-MacBook-Pro:chess.js andrew$ git checkout master; npm test 2>&1 | grep "Tests:"
Previous HEAD position was 426e7ed 0.13.2
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
Tests:       339 passed, 339 total

@jhlywa
Copy link
Owner Author

jhlywa commented Jun 10, 2022

Hi @DevAndrewGeorge. No, I haven't been able to account for the discrepancy in tests cases. I did omit a few cases that I though were redundant on the dev branch, and also refactored a few others. I expected to be within 10-20 tests cases of v0.13.2, but running your snippet on dev shows we're missing about 100 cases. (BTW - I think you ran on the wrong branch, be sure you're on dev when generating a test count)

$ git checkout dev && npm test 2>&1 | grep "Tests:"
Tests:       237 passed, 237 total

@DevAndrewGeorge
Copy link
Contributor

Ah, good catch. The main source of your discrepancy is that the validate_fen.test.ts file ran all cases in a single test. See #333 where I've "fixed" this in by running each case individually.

@LandonSchropp
Copy link

LandonSchropp commented Jul 16, 2022

If there's a major version bump coming anyway, can I suggest the accessor functions that take no arguments be converted to properties? For example:

fen() {
   ...
}

Could become:

get fen() {
  ...
}

@LandonSchropp
Copy link

LandonSchropp commented Jul 16, 2022

Another request:

Would it be possible to include a few more properties for the' history' function? Currently, I'm adding these to the history property in my wrapper:

  • fen
  • check
  • checkmate

Edit: I'm withdrawing my original request. After some more thought, I realized that the move history should be differentiated from the position history. In my wrapper, I split this into two properties:

  • Chessboard#moves: Returns the history of the moves the player made.

    interface Move {
      player: Player,
      from: Square,
      to: Square,
      piece: Piece,
      algebraic: string,
      capture: boolean,
    }
  • Chessboard#positions: Represents the positions in the game, including the starting position. The properties of the position are a result of a move, but not part of the move itself.

    interface Position {
      fen: string,
      check: boolean,
      checkmate: boolean,
      comment: string | null,
      highlights: Highlight[]
      arrows: Arrow[]
    }

I think this differentiation is an example of the Fencepost Problem, where the positions represent the posts and the positions represent the panels between them.

I don't know if this differentiation is helpful to anyone else, but creating it has made using my wrapper much, much simpler.

@SheetJSDev
Copy link

Improve FEN validation. It should detect common hand-entered such as missing kings and incorrect castling rights.

If missing kings is considered invalid, extra kings should also be considered invalid. kkkkkkkk/8/8/8/8/8/8/KKKKKKKK w KQkq - 0 1 is an example board discussed in issue #289

@will-lamerton
Copy link

Wicked stuff! Using Chess.js in our Chess engine! Gave the package credit for move generation and game state detection on the website and the in the docs :)

@vicary
Copy link

vicary commented Sep 23, 2022

Never use all caps for abbv. / acronyms, they tend to mess with other things.

@jhlywa
Copy link
Owner Author

jhlywa commented Jan 14, 2023

Hi all. The latest beta has been pushed to NPM. I have one more planned code change (adding a before and after FEN field to to verbose history output). Here are the release notes:

https://github.com/jhlywa/chess.js/releases/tag/v1.0.0-beta.0

@SheetJSDev
Copy link

There's a packaging issue with the beta:

npm ERR! command failed
npm ERR! command sh -c -- npm run build
npm ERR! > [email protected] build
npm ERR! > tsc --build
npm ERR! sh: tsc: command not found

It looks like the module ships with the typescript code (src/chess.ts) as well as the JS code (dist/chess.js and types file) but also has a postinstall step that runs tsc. Since main points to the JS file in the dist folder, is that post-install step necessary?

@jhlywa
Copy link
Owner Author

jhlywa commented Jan 15, 2023

@SheetJSDev Doh! Thanks for the feedback. A fix tagged 1.0.0-beta.1 has been published.

@qwerty084
Copy link

Hi all. The latest beta has been pushed to NPM. I have one more planned code change (adding a before and after FEN field to to verbose history output). Here are the release notes:

https://github.com/jhlywa/chess.js/releases/tag/v1.0.0-beta.0

This change would be really nice, having before and after fen :)

@jhlywa
Copy link
Owner Author

jhlywa commented Mar 18, 2023

@qwerty084 before and after have been added to the Move object returned by .move(), .undo, .moves(), and .history(). The two latter methods need to be called with { verbose: true } to return Move[].

It's been pushed to npm as 1.0.0-beta.4

@neofight78
Copy link
Contributor

@jhlywa "moves() should work even if there are no kings on the board." can be ticked off this list now :)

@arthurchumak
Copy link

🥺 waiting for chess 960 support #122

@ppeloton
Copy link

I am interested in the support for null moves and I have already made some thoughts how to implement such a feature to this package. Before I provide a pull request, have there been any thoughts on the SAN of a null move. There is no standard SAN representation for a null move. I would suggest "--" but there are others (see e.g. https://chess.stackexchange.com/questions/14072/san-for-nullmove )

@jhlywa
Copy link
Owner Author

jhlywa commented Mar 17, 2024

Hi @ppeloton. The lack of a standard is why we traditionally didn't support null moves. However, with switch to the permissive SAN parser, I think it's time we add it. I think it's reasonable to start with "--" as the SAN representation of a null move.

ppeloton added a commit to ppeloton/chess.js that referenced this issue Mar 18, 2024
@ppeloton
Copy link

@jhlywa : I made a pull request with an implementation for SAN "--" and I also added a test case. I am happy to discuss any necessary changes / further enhancements. I think the implementation is quite straight forward, however when making a null move a dummy move is created (black king moves from a8 to a8). It is not really visible to a user, only if you make the chess.history({ verbose: true }) command. Maybe someone has a better idea how to implement this part.

ppeloton added a commit to ppeloton/chess.js that referenced this issue Mar 22, 2024
ppeloton added a commit to ppeloton/chess.js that referenced this issue Mar 23, 2024
ppeloton added a commit to ppeloton/chess.js that referenced this issue Mar 23, 2024
@cmgchess
Copy link

looking forward 960 support!

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