phetsims / phetsims/rosetta

Improve typing and code structure

Open
#469 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

priority:5-deferred
Dominant language
TypeScript
Stars
3
Forks
3
PR merge metrics
No merged PRs in 30d

Description

Under #311 Rosetta was converted from JavaScript to TypeScript. This has made the code much more maintainable given that we now have strong type checking and much better IDE support for code navigation. As I worked on this, I noticed a number of things that could be done to improve the code structure to make it more maintainable, but we just don't have the time to work on this right now. I thought I'd capture some thoughts in a GitHub issue in case there was ever time and funding in the future to work on this. Here's my list:

  • The data for the English strings (the ones that live in the sim and common-code repos), the translated strings (the ones that live in the babel repo), the translation form data, and the translation report are manipulated and tweaked a lot by the code and they move through it, which has left us with a lot of similar but slightly different type definitions. This is sort of a stylistic thing between JS and TS - there is no strong typing in JS, so developers tend to change the structure of the data as it moves around. It's a bit much in TS though, and we would be better served to convert the data to one format when it comes in to the code, and we could have fewer types.
  • The server-side code is very procedural as opposed to object-oriented. I think this is due to the nature of an Express app - the routes are set up the call functions, so breaking it down as a bunch of functions seemed natural. However, there are a number of places that could benefit from a more object-oriented design so that we can have information and functionality encapsulated. The translation report is, to me, the most obvious place where this would have benefit, but I think it could also be used for encapsulating the sets of strings as the move about in the code.
  • The Record type is used a lot. This was a natural fit given the way the original JS code worked since many of the stringKey-stringValue sets were done as objects. However, we don't use Record much in the PhET sim code, so it's not particularly familiar to other devs, and it seems a bit forced to me in some places in the Rosetta code. It may be more clear and familiar to use the Map type in these places.

I'll defer this issue to start with, and perhaps we will get back to it in the future sometime.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

The issue identifies three broad areas: normalizing translation data and types, restructuring server-side Express code around the translation report, and replacing some Record uses with Map. Start by surveying the relevant Rosetta code for one area and its existing tests; because no files or tests are named, the work is not ready until a concrete scope and completion criteria are agreed.

Written by the indexing model from the issue text.

Assessment

Tech stack
express, typescript
Domain
backend, localization, tooling
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.