chore: merge shared code between ui and app by OGPoyraz · Pull Request #482 · MetaMask/metamask-desktop

OGPoyraz · 2023-02-01T12:15:58Z

Overview

This PR aims to create a shared folder under src and merge the common code between UI and app.
Common code consists of:

Constants
- Links
- Locale indexes
- Theme indexes
- UI Routes
- Metric constants for analytics (Not going to add them for now since topic is under progress)
Locales
- locale jsons
- locale helpers

OGPoyraz · 2023-02-01T12:18:28Z

packages/app/.eslintrc.js

    },
    {
-      files: ['src/ui/**/*.{js,ts,jsx,tsx}'],
+      files: ['src/ui/**/*.{js,ts,jsx,tsx}', 'src/shared/**/*.{js,ts,jsx,tsx}'],


shared folder to be linted with UI rules.

OGPoyraz · 2023-02-01T12:22:05Z

packages/app/src/app/desktop-app.ts

    });

+    ipcMain.handle('set-language', (_event, language) => {
+      this.Translation.setLanguage(language);


Language changes need to be synced with main process, reasons explained in Translation class below.

OGPoyraz · 2023-02-01T12:23:19Z

packages/app/src/app/utils/translation.ts

+  private language: string;
+
+  constructor() {
+    this.language = readPersistedSettingFromAppState({


We read language from UI only once the user executes the app.

OGPoyraz · 2023-02-01T12:25:22Z

packages/app/src/app/utils/translation.ts

+    this.translate = memoize(this.translate);
+  }
+
+  public setLanguage(language: string) {


Reading UI state is an expensive operation from performance perspective, this is why we are syncing language here.
So subsequent language changes in the UI will call electron bridge set-language method and eventually will trigger this method.

OGPoyraz · 2023-02-01T19:26:40Z

.circleci/config.yml

      - checkout_with_submodule
      - restore_cache:
-          key: dependency-cache-v4-{{ checksum "yarn.lock" }}
+          key: dependency-cache-v6-{{ checksum "yarn.lock" }}


v5 is accidentally cached wrong on my earlier commits

Why are we changing the version here at all if we haven't changed the pipeline at all?

My mistake fixed.

matthewwalsh0 · 2023-02-02T13:34:43Z

packages/app/src/app/desktop-app.ts

    super();
    this.metricsService = MetricsService;
    this.UIState = UIState;
+    this.Translation = Translation;


As this is a singleton, we shouldn't need a property here as we can just use the import directly.

matthewwalsh0 · 2023-02-02T13:39:06Z

packages/app/src/app/ui/app-navigation.ts

  private createShowMenuItem() {
    return {
-      label: 'Show',
+      label: this.Translation.translate('show'),


For the sake of simplicity, we could remove the class from the Translation file, use a standard let for the language, and export the methods directly meaning we could import it here as t and make all these translations less verbose.

matthewwalsh0 · 2023-02-02T13:40:50Z

packages/app/src/app/update-check.ts


-export const updateCheck = async (): Promise<UpdateCheckResult | null> => {
+export const updateCheck = async (
+  translate: (key: string) => any,


As the translations are so fundamental and not conceptually related, I'd recommend just importing the file directly here rather using a constructor argument, given it's a singleton so there is nothing unique about the instance.

matthewwalsh0 · 2023-02-02T13:41:48Z

packages/app/src/app/utils/translation.ts

+import { locales } from '../../shared/locales';
+import { LOCALE_KEYS } from '../../shared/constants/locale';
+
+class Translation {


As mentioned in the other comment, I'd not use a class here but just store the language in a let as that would allow the other files to import just the translate method with a compact alias like t.

Make sense, then we would keep the same naming on both UI and app 👍

matthewwalsh0 · 2023-02-02T13:43:10Z

packages/app/src/shared/constants/links.js

@@ -0,0 +1,5 @@
+export const mmdSubmitTicket =


Capitals for all constants so URL_SUBMIT_TICKET and URL?

matthewwalsh0 · 2023-02-02T13:43:30Z

packages/app/src/shared/constants/theme.js

+  OS: 'os',
+};
+
+export const themeIndex = [


Capitals here also.

Fixed this and others

socket-security · 2023-02-03T08:12:01Z

Socket Security Pull Request Report

👍 No new dependency issues detected in pull request

Pull request report summary

Issue	Status
Install scripts	✅ 0 issues
Native code	✅ 0 issues
Bin script confusion	✅ 0 issues
Bin script shell injection	✅ 0 issues
Unresolved require	✅ 0 issues
Invalid package.json	✅ 0 issues
HTTP dependency	✅ 0 issues
Git dependency	✅ 0 issues
Potential typo squat	✅ 0 issues
Known Malware	✅ 0 issues
Telemetry	✅ 0 issues
Protestware/Troll package	✅ 0 issues

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@2.4.2

Powered by socket.dev

OGPoyraz requested a review from a team February 1, 2023 12:15

OGPoyraz force-pushed the chore/merge-shared-code-between-ui-and-app branch from c093a9c to c462c48 Compare February 1, 2023 12:16

OGPoyraz commented Feb 1, 2023

View reviewed changes

OGPoyraz force-pushed the chore/merge-shared-code-between-ui-and-app branch 12 times, most recently from 8c2e161 to e9b97b3 Compare February 1, 2023 14:25

OGPoyraz commented Feb 1, 2023

View reviewed changes

matthewwalsh0 reviewed Feb 2, 2023

View reviewed changes

OGPoyraz force-pushed the chore/merge-shared-code-between-ui-and-app branch 3 times, most recently from b182002 to 036e663 Compare February 2, 2023 18:09

OGPoyraz force-pushed the chore/merge-shared-code-between-ui-and-app branch from 40043bf to 12f772d Compare February 3, 2023 08:15

OGPoyraz force-pushed the chore/merge-shared-code-between-ui-and-app branch 2 times, most recently from 80f18e0 to 63a76f4 Compare February 3, 2023 08:40

chore: merge shared code between ui and app

e72930f

OGPoyraz force-pushed the chore/merge-shared-code-between-ui-and-app branch from 5e64573 to e72930f Compare February 6, 2023 10:15

Merge branch 'main' into chore/merge-shared-code-between-ui-and-app

64ddf12

matthewwalsh0 approved these changes Feb 6, 2023

View reviewed changes

OGPoyraz merged commit 941f526 into main Feb 6, 2023

cryptotavares mentioned this pull request Mar 1, 2023

Release/app 0.0.4 #564

Merged

Uh oh!

Conversation

OGPoyraz commented Feb 1, 2023

Overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OGPoyraz Feb 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OGPoyraz Feb 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OGPoyraz Feb 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewwalsh0 Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

socket-security bot commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

OGPoyraz Feb 1, 2023 •

edited

Loading

OGPoyraz Feb 1, 2023 •

edited

Loading

OGPoyraz Feb 1, 2023 •

edited

Loading

matthewwalsh0 Feb 2, 2023 •

edited

Loading

socket-security bot commented Feb 3, 2023 •

edited

Loading