feat(js/core): added middleware to reflection api#5044
feat(js/core): added middleware to reflection api#5044pavelgj wants to merge 11 commits intopj/gm-impl-middlewarefrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request extends the ReflectionServer to support the 'middleware' type in the /api/values endpoint and implements logic to invoke toJson on returned values when available. It also adds a new test suite to verify the reflection API. A potential runtime error was identified in the mapping logic where a nullish check is required before accessing properties on values.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces toJson mapping for listValues in both ReflectionServer and ReflectionServerV2, allowing for custom serialization of registered values. It also adds support for the middleware type in the REST API and includes comprehensive tests. Review feedback identifies a breaking change in the REST API where the response format was changed from a map to an array, resulting in data loss and inconsistency with the WebSocket implementation. Additionally, the reviewer recommended making the supported type validation more dynamic to match the flexibility of the V2 server.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the reflection servers to support the 'middleware' type and adds logic to map values using their 'toJson' method if available. The reviewer identified that the newly introduced strict validation for the 'type' parameter in the V2 reflection server is overly restrictive and causes a regression by preventing access to other valid types. It is recommended to remove this strict validation to allow the backend to handle type checking, which would also necessitate updating the corresponding test case.
Checklist (if applicable):