Investigate using tsarch to validate architecture constraints in frontend

[mathjazz]

This issue was created automatically by a script.

Bug 1712507

Bug Reporter: @Pike CC: @abowler2, adrian@gaudebert.fr Depends on: Bug 1685565

There's a tool called tsarch which allows to test things like dependencies etc that should come with an architecture.

We should investigate if we have such constraints, and if so, if they're reasonable to validate automatically.

I've made up two potential rules, but I'm not sure if they're actually any good:

• Things in src/core/** should not depend on things in src/modules/**
• There shouldn't be circular dependencies between high-level features

Both of these can be implemented in tsarch, and, both of them fail. I'll draft a PR to have something to talk about.

[mathjazz]

Comment Author: GitHub Bugzilla PR Linker <pulgasaur@mozilla.bugs>

Created attachment 9223136 Link to GitHub pull-request: https://github.com/mozilla/pontoon/pull/1951

Attached file: file_650808458.txt (text/x-github-pull-request, 44 bytes) Description: Link to GitHub pull-request: https://github.com/mozilla/pontoon/pull/1951

[mathjazz]

Comment Author: @Pike

To add some context, I looked at the feature cycles, trying to figure out what my speculations mean in practice, and if the cases that trigger them are bugs or features or nits.

First, core/api -> core/locale -> core/api.

This is triggered by core/api/machinery taking Locale as input. And the GraphQL-based types are defined in core/*/actions, for both Locale and Project. In the case of Locale, the data is even processed for cldrPlurals. So we have a GraphQL version of the Locale type going from core/api/locale to core/locale, and then core/api/machinery taking the processed type from core/locale.

Bug or feature or nitpicking?

My take would be to move the definition of Locale and Localization from core/locale/actions into core/api/types, and the processing of the GraphQL response into core/api/locale and /project. Maybe have a generic GraphQL handler even in core/api/base.

Just briefly glanced at core/editor -> modules/history -> core/editor. One trigger here seems to be

const historySelector = (state): HistoryState => state[history.NAME];

in core/editor/selectors.

Bug or feature or nitpicking?

The reverse dependency is that modules/history/actions uses core/editor/actions to change the status of prior translations. I kinda think that's OK.

My take would be to have a core/history feature with both state and selector. No idea about actions, the UPDATE one is the one that circles, so I'd say that action for sure not?

Big picture, is this worth it? For one, both, either?

[mathjazz]

Comment Author: @Pike

°tsarch° currently isn't maintained, https://twitter.com/ArghRich/status/1420719372641914883 says

There‘s currently no maintainer. Maybe that‘ll change in a few months because my next project is Ts again and I plan to use it widely.

Might still be interesting to look at the rules and violations of them to see what our frontend arch is, or should be. But maybe the PR isn't useful to actually merge.