Move to TypeScript #955

karaggeorge · 2020-11-05T17:20:17Z

Closes #984
Closes #895

This is a PR with a lot of changes. It started as rewriting the editor, in which I started writing the new code in TS (since next supports it pretty seamlessly), which then led to start turning the main process in TS as well. Since it was a big rewrite I also started making other big changes to re-organizing some things. I'll try to sum up the biggest things below since it's a lot of changes to review at once with no context:

Keep in mind that this is very much WIP, so there's a lot of duplicate files that I haven't fully transitioned, and a lot of debug code and console.logs, but I just wanted to get some eyes on it early.

General

TS on both renderer/main
TS common types are duplicated for now as I haven't figured out a way to import them in both without transpiler breaking. But I've left comments on the types that should eventually only live in one place
Shared TS types are achieved via a Symlink of main/common -> renderer/common

Renderer

New common top-level component in _app.tsx, which:
- Disables SSR so we can safely use electron/electron-util
- Automatically applies base styles + other utilities like dark mode
- Hooks for window args (more info below)
- Common functionality for showing the window once initial render has finished
Every new file (or rewritten) is using function exports (instead of Component classes), TS instead of PropTypes and hooks
Every new page (currently just editor) uses unstated-next instead of unstated v1
Hooks for the new "remote-state" which will be explained below in more detail

Main

Full TS rewrite
0 circular dependencies
KapWindow class which:
- Extends Browser window and adds common utility we have in all our windows, like next.js route, logic for showing the window when it mounts and now a common way to pass initial window args/state
- It needs to be renamed to BrowserWindow, as electron internally has checks like window.constructor.name === 'BrowserWindow'
- Handles Dock/Menu for the windows that use it
WindowManager which incorporates all window actions to avoid the numerous circular dependecies
New "remote-states" (more details below)
Conversion logic (ffmpeg commands) rewritten in TS and refactored to expect new source encodings
New logic for removing audio, doing it in the same conversion instead of running one conversion just to remove the audio
Plugin system rewritten in TS, and refactored some logic to make plugins less dangerous and causing less bugs when a plugin has an issue loading (wip)
New classes like Video/Recording/Conversion that depend on each other to make options passing easier and more concrete, instead of passing random objects with 15 properties between every class/window and duplicating utility code (like default naming, encoding, fps etc)

Tests/Linting

XO set up to work with both the JS and the TS files
XO also set up to have a separate set of rules for test files (and mocks)
AVA set to work with typescript (using ts-node)
Mocks rewrote to work with Typescript
All tests rewrote in TS

Remote States

Not sure if that's the best name, but the way I thought of this was firstly for export-options. So the options that populate the editor window (plugins/fps etc) need to be synced, since you can install/uninstall plugins as you have the editor open. This was done in a pretty ugly way, so I wanted something that'd work better with hooks on the renderer side. Then I saw that it could be used in general, so I made it abstract. The way it works, is on the main side, we need a function that takes an argument sendState which it can use to notify that state has changed, and returns an object with two properties. One is getState that a new subscription can use to receive the state at any given point, and an object actions who's properties are functions that can change the state. Then on the renderer side, via a hook, the page can subscribe to one of these states (by name) and they will receive a state automatically updating whenever needed, a method to manually refresh the state and all the actions exported by main as proxy functions, that when called will actually just call the main ones. This is all done in the background with the two remote-state implementations that communicate via ipc and handle subscriptions/state updates/actions. To write new states, we simply write the function explained above for main and call the hook in the renderer. TS should be able to auto generate types for renderer based on the main implementation, but since we don't have common types yet, I needed to replicate the main type to the renderer to get that to work.

The only implementation for the above so far is the editor-options which handles, plugins (share/edit), and fps usage, but I'm planning on using one for conversions and keeping track (since now both editor windows and exports window will need to subscribe) and then for the cropper/action-bar to communicate with each other when I rewrite the cropper.

Any feedback/discussion is welcome. This is the first draft of all of this, and I'm trying to make sure I do it in a way that will help development in the future. Mostly trying to abstract things we do in every window, but slightly differently and it gets hard to manage/update.

karaggeorge · 2020-11-05T17:22:06Z

main/converters/index.ts

+
+const converters = new Map([
+  [Encoding.h264, h264Converters]
+]);


When we support recording in hevc, we can add converters for that here if they are different from h264 (same for ProRes etc)

karaggeorge · 2020-11-05T17:24:47Z

main/plugins/config.ts

+  constructor(name: string, services: Service[]) {
+    const defaults = {};
+
+    const validators = services


One thing I noticed here, but hasn't come up as an issue yet, is that we allow each service exported by a plugin to have its own config schema object, but we actually save all of them (and validate them) against the same object (electron-store instance). So the plugins need to make sure their different services don't have conflicting field schemas

karaggeorge · 2020-11-05T17:25:59Z

main/plugins/service.ts

+  didStopRecording?: () => {};
+  willEnable?: () => {};
+  cleanUp?: () => {};
+}


We should find a way (when this is done) to export this somehow as a package so plugins written in TS can leverage these types

karaggeorge · 2020-11-05T17:28:07Z

main/video.ts

+      if (this.encoding === 'h264') {
+        this.previewPath = this.filePath;
+      } else {
+        this.previewPath = await convertToH264(this.filePath);


Written this like that (with a separate promise) as in the future when we have ProRes that takes a while to generate a preview, we can show the editor early with a spinner saying that the preview is being generated.

karaggeorge · 2020-11-05T17:28:25Z

package.json

-    "extends": "xo-react",
+    "extends": [
+      "xo-react",
+      "xo-typescript"


Need to remove this

sindresorhus · 2020-11-20T03:36:30Z

main/common/settings.ts

@@ -114,26 +138,27 @@ if (store.has('recordKeyboardShortcut')) {

 // TODO: Remove this when we feel like everyone has migrated
 if (store.has('cropperShortcut')) {
-  store.set('shortcuts.triggerCropper', shortcutToAccelerator(store.get('cropperShortcut')));
+  // TODO: Investigate type for dot notation


This is not yet possible, AFAIK.

sindresorhus/type-fest#134 (comment)

I thought I saw something like this on Twitter a few months ago. It might have been something that hadn't landed in a stable version yet. It's not a big deal for us, but figured we could look into it

sindresorhus · 2020-11-20T03:38:56Z

main/plugins/plugin.ts

+  }
+
+  get shareServices() {
+    return this.content.shareServices || [];


Suggested change

return this.content.shareServices || [];

return this.content.shareServices ?? [];

?

sindresorhus · 2020-11-20T03:49:13Z

From your description, this sounds like a huge improvement on every level. I would recommend targeting just getting something working first, merge this PR, and then do follow-up PRs that perfect/abstract it and turn more things into TS.

Planning on adding something like a Menu for a window, and this class takes care of updating the application menu based on the window being focused/blurred

👍 This would be much closer to how native apps work.

karaggeorge · 2021-02-11T00:22:04Z

.circleci/config.yml

@@ -6,7 +6,7 @@ jobs:
    steps:
      - checkout
      - run: yarn
-      - run: yarn test


Disabling this so I can still get PR builds until I go back and clean up linting and tests

sindresorhus · 2021-02-15T17:51:48Z

The new export window looks hot 🙌

If I right-click the menu bar icon and click "About", it doesn't show. Only when I click the icon normally, it then shows.
The menu opens slowly when right-clicking the menu bar icon. And sometimes it doesn't show.
When right-clicking the menu bar icon and choosing "Open Video", the open panel is not focused. You should call App.activate (or whatever it is) before opening the panel.
In the editor, the play/pause button doesn't work.
In the editor, the FPS slider lags when moving it.
In the editor, the "Open in…" item should be "Open In…".
In the export window, the back button doesn't have a "click" state, only hover. It should have a click state, but not hover state.
In the export window: Initiating… => Initializing…
In the export window, we need to indicate to the user that the thumbnail can be dragged. Either through an icon or a popover that shows the first time.
The export window should not have a maximize window button.
When clicking the "red" window close button in the export window, it closes the export right away without any warning. I think we should tell the user that it will continue exporting and be available in the "Export History".
In the export window, when hovering the center "X" button, the mouse pointer should not be a hand.
Pressing Esc key in the export window should have the same effect as clicking the "X" button (in the center).
In the export window, clicking the cancel button "X" causes an unhandled rejection:

Unhandled Promise Rejection
CancelError: Promise was canceled
    at PCancelable.cancel (/Applications/Kap.app/Contents/Resources/app.asar/node_modules/p-cancelable/index.js:95:17)
    at Export.onCancel (/Applications/Kap.app/Contents/Resources/app.asar/dist-js/export.js:53:73)
    at Conversion.cancel (/Applications/Kap.app/Contents/Resources/app.asar/dist-js/conversion.js:85:79)
    at Object.cancel (/Applications/Kap.app/Contents/Resources/app.asar/dist-js/remote-states/conversion.js:43:102)
    at /Applications/Kap.app/Contents/Resources/app.asar/dist-js/remote-states/setup-remote-state.js:41:83
    at IpcMainImpl.listener (/Applications/Kap.app/Contents/Resources/app.asar/node_modules/electron-better-ipc/source/main.js:90:28)
    at WebContents.<anonymous> (electron/js2c/browser_init.js:3851:15)`

In the preferences window, changing the "capture frame rate" is slow. It only updates the dropdown a second after changing it.
When editing the config for a plugin, clicking "Open config file" doesn't do anything.

sindresorhus · 2021-02-15T11:30:22Z

main/common/types/remote-states.ts

+  title: string;
+  pluginName: string;
+  pluginPath: string;
+  apps?: App[];


We don't lint this yet as the linting is buggy, but generally use readonly whenever possible

Do you mean add readonly to all fields? Maybe wrap the types with Readonly?

sindresorhus · 2021-02-15T11:33:17Z

main/converters/index.ts

+      croppedPath = options.inputPath;
+    }
+
+    let canceled = false;


Suggested change

let canceled = false;

let isCanceled = false;

sindresorhus · 2021-02-15T11:34:12Z

main/converters/utils.ts

+  };
+}
+
+export const makeEven = (n: number) => 2 * Math.round(n / 2);


Suggested change

export const makeEven = (n: number) => 2 * Math.round(n / 2);

export const makeEven = (number: number) => 2 * Math.round(number / 2);

sindresorhus · 2021-02-15T11:41:19Z

tsconfig.json

+    "target": "es2019",
+    "module": "commonjs",
+    "esModuleInterop": true,
+    "downlevelIteration": true,


Why enable this?

Allows stuff like [...someMap.values()] without @ts-ignore

I think that's only true when the target is ES5. For modern targets, that should work without downlevelIteration as it just uses it natively.

sindresorhus · 2021-02-15T11:42:57Z

renderer/tsconfig.json

@@ -0,0 +1,41 @@
+{
+  "compilerOptions": {
+    "target": "es5",


Why is the target es5? That will include a lot of polyfill bloat we don't need.

Not sure, this is autogenerated by Next.js
What should the target be?

Same as the main process tsconfig.

skllcrn

We should get rid of the warning when navigating back from an ongoing conversion to the editor. I'll often realise I need to make further changes after starting a conversion, and the dialog slows me down unnecessarily, I only need to be warned if I'm about to lose the source file.

karaggeorge · 2021-03-09T23:07:13Z

Unhandled Promise Rejection
CancelError: Promise was canceled
    at PCancelable.cancel (/Applications/Kap.app/Contents/Resources/app.asar/node_modules/p-cancelable/index.js:95:17)
    at Export.onCancel (/Applications/Kap.app/Contents/Resources/app.asar/dist-js/export.js:53:73)
    at Conversion.cancel (/Applications/Kap.app/Contents/Resources/app.asar/dist-js/conversion.js:85:79)
    at Object.cancel (/Applications/Kap.app/Contents/Resources/app.asar/dist-js/remote-states/conversion.js:43:102)
    at /Applications/Kap.app/Contents/Resources/app.asar/dist-js/remote-states/setup-remote-state.js:41:83
    at IpcMainImpl.listener (/Applications/Kap.app/Contents/Resources/app.asar/node_modules/electron-better-ipc/source/main.js:90:28)
    at WebContents.<anonymous> (electron/js2c/browser_init.js:3851:15)`

In the preferences window, changing the "capture frame rate" is slow. It only updates the dropdown a second after changing it.
When editing the config for a plugin, clicking "Open config file" doesn't do anything.
We should get rid of the warning when navigating back from an ongoing conversion to the editor. I'll often realise I need to make further changes after starting a conversion, and the dialog slows me down unnecessarily, I only need to be warned if I'm about to lose the source file.

karaggeorge · 2021-03-09T23:11:07Z

Keep in track of the ones I implemented above ^
Some questions for some others:

The menu opens slowly when right-clicking the menu bar icon. And sometimes it doesn't show.

The only thing we do async before opening the menu is getting the input devices. The only way around I can think is instead of checking them on every menu open, checking them every x seconds and using the last value. Surprisingly when running locally this is not delayed for me at all

I'll investigate more

In the export window, we need to indicate to the user that the thumbnail can be dragged. Either through an icon or a popover that shows the first time.

What would the icon be that would indicate that? Popover I think would also work, maybe make it a more abstract thing that we can re-use to introduce new or hidden features in general? But maybe different issue/PR?

When clicking the "red" window close button in the export window, it closes the export right away without any warning. I think we should tell the user that it will continue exporting and be available in the "Export History".

Should this be a dialog (blocking), or close immediately, and just shoot a notification? Only thing about notification is the user might not see if it they have notifications off (or permission not granted)

cc @skllcrn as well

sindresorhus · 2021-03-10T04:34:39Z

The only thing we do async before opening the menu is getting the input devices.

Why are we getting the input devices? They don't show in the menu. Could we not fetch those lazily?

Should this be a dialog (blocking)

Dialog, but we only need to show it once (ever).

What would the icon be that would indicate that? Popover I think would also work, maybe make it a more abstract thing that we can re-use to introduce new or hidden features in general? But maybe different issue/PR?

Sure: #985

karaggeorge · 2021-03-10T15:28:36Z

For this submenu:

When you say lazily, do you mean show the menu and fill them later keeping a reference? Like making the submenu disabled until we fetch?

If they don't show in the menu you might need the microphone permission. Usually we ask that when the cropper opens and we don't have it.

sindresorhus · 2021-03-10T16:57:34Z

When you say lazily, do you mean show the menu and fill them later keeping a reference? Like making the submenu disabled until we fetch?

Yes, but not sure whether that's possible in Electron.

If they don't show in the menu you might need the microphone permission. Usually we ask that when the cropper opens and we don't have it.

I forgot it was supposed to show there. It doesn't for me. I don't think I got a prompt either. And Kap doesn't show in the System Preferences for Microphone access.

Here's the menu I see:

karaggeorge · 2021-03-10T17:00:16Z

Hmm maybe some kind of different permission thing for macos 11? I think we use the electron api for microphone permission. Did they make an update to it to handle macos 11 in a newer electron version?

Electron 12 I think just released and it also has the share menu stuff, I can try to upgrade.

If it's not the input devices, I don't know what would delay the menu. Let me reset my permission and try again see if there's something else going on

sindresorhus · 2021-03-10T17:03:09Z

If it's not the input devices, I don't know what would delay the menu. Let me reset my permission and try again see if there's something else going on

It's not a big deal though. Don't let it block this PR. I can open an issue about it afterwards.

karaggeorge · 2021-03-12T00:39:58Z

@sindresorhus everything should be done, except for the one you opened an issue for, and then these:

The menu opens slowly when right-clicking the menu bar icon. And sometimes it doesn't show.

Will try to figure out in a separate PR, probably need to upgrade Electron from 8.x

In the preferences window, changing the "capture frame rate" is slow. It only updates the dropdown a second after changing it.

Right now it's the fastest it'll be, I think it's because of an ipc call every time? (I didn't change it at all) but since I'm about to rewrite Preferences window soon, I'd rather address in that PR

So, let's test this a bit more, and if we don't have any other issues for a bit, we can merge

sindresorhus

It's working great for me. I think we can merge this sooner than later.

skllcrn

Fantastic work @karaggeorge! A couple of things I've noticed:

We seem to have lost the file menu option to save the original
File names in the conversion screen appear black when the system is in light mode
Also in light mode, the window background turns light when transitioning back to the editor
When navigating back to the editor, the trimming end is lost
Hitting enter while editing the FPS does not validate the input

Have a lovely weekend ❤️

karaggeorge · 2021-03-12T19:22:05Z

@skllcrn good catches, and although you forced me to go to light mode which throws me off so much, everything is fixed 😄

Forgot our editor is perma-dark styled, so was using the theme-dependent css vars

sindresorhus · 2021-03-13T10:08:10Z

This is such a nice improvement to the code quality 👌

karaggeorge added 3 commits October 18, 2020 19:01

WIP

3289337

Merge branch 'master' into editor-v2

79aafe2

WIP

b73a37a

karaggeorge added the Do Not Merge label Nov 5, 2020

karaggeorge requested review from sindresorhus and albinekb November 5, 2020 17:20

karaggeorge commented Nov 5, 2020

View reviewed changes

package.json Outdated

"extends": "xo-react",

"extends": [

"xo-react",

"xo-typescript"

Copy link

Member Author

karaggeorge Nov 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to remove this

sindresorhus suggested changes Nov 20, 2020

View reviewed changes

Base automatically changed from master to main January 23, 2021 13:41

Get build for testing

f2a05ca

karaggeorge commented Feb 11, 2021

View reviewed changes

Finish the main process TS rewrite

a2d0d65

karaggeorge marked this pull request as ready for review February 14, 2021 20:44

karaggeorge requested a review from sindresorhus February 14, 2021 20:44

Fix build errors

5b84c16

sindresorhus suggested changes Feb 15, 2021

View reviewed changes

sindresorhus changed the title ~~TypeScript Rewrite~~ Move to TypeScript Feb 15, 2021

skllcrn requested changes Mar 9, 2021

View reviewed changes

Update menu behavior, editor UX fixes, tsconfig and rule changes

ac3900f

karaggeorge mentioned this pull request Mar 11, 2021

Using mp4(h.264) as default export format instead of av1 #984

Closed

karaggeorge added 3 commits March 11, 2021 18:58

Fix more issues

d1b21fc

Add keyboard action hook

c8e29ff

Add flags, fix isOnline call and fix config issue

a9af816

Fix electron-util import

2e35370

sindresorhus mentioned this pull request Mar 12, 2021

Menu bar item menu opens slowly sometimes #988

Open

sindresorhus approved these changes Mar 12, 2021

View reviewed changes

skllcrn requested changes Mar 12, 2021

View reviewed changes

karaggeorge added 4 commits March 12, 2021 13:49

Fix Save Original menu item missing

c4d2902

Fix light mode styles

8379df2

Fix fps validation on Enter

bbfe28d

Fix end handle on editor reseting after conversion

c9098eb

sindresorhus merged commit 8bd89ce into main Mar 13, 2021

sindresorhus deleted the editor-v2 branch March 13, 2021 10:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move to TypeScript #955

Move to TypeScript #955

karaggeorge commented Nov 5, 2020 •

edited

Loading

karaggeorge Nov 5, 2020

karaggeorge Nov 5, 2020

karaggeorge Nov 5, 2020

karaggeorge Nov 5, 2020

karaggeorge Nov 5, 2020

sindresorhus Nov 20, 2020

karaggeorge Nov 20, 2020

sindresorhus Nov 20, 2020

sindresorhus commented Nov 20, 2020

karaggeorge Feb 11, 2021

sindresorhus commented Feb 15, 2021

sindresorhus Feb 15, 2021

karaggeorge Mar 9, 2021

sindresorhus Feb 15, 2021

sindresorhus Feb 15, 2021

sindresorhus Feb 15, 2021

karaggeorge Feb 15, 2021

sindresorhus Feb 16, 2021 •

edited

Loading

sindresorhus Feb 15, 2021

karaggeorge Feb 15, 2021

sindresorhus Feb 16, 2021

skllcrn left a comment

karaggeorge commented Mar 9, 2021 •

edited

Loading

karaggeorge commented Mar 9, 2021 •

edited

Loading

sindresorhus commented Mar 10, 2021

karaggeorge commented Mar 10, 2021

sindresorhus commented Mar 10, 2021

karaggeorge commented Mar 10, 2021

sindresorhus commented Mar 10, 2021

karaggeorge commented Mar 12, 2021

sindresorhus left a comment

skllcrn left a comment •

edited

Loading

karaggeorge commented Mar 12, 2021 •

edited

Loading

sindresorhus commented Mar 13, 2021

	return this.content.shareServices \|\| [];
	return this.content.shareServices ?? [];

	export const makeEven = (n: number) => 2 * Math.round(n / 2);
	export const makeEven = (number: number) => 2 * Math.round(number / 2);

Move to TypeScript #955

Move to TypeScript #955

Conversation

karaggeorge commented Nov 5, 2020 • edited Loading

General

Renderer

Main

Tests/Linting

Remote States

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Nov 20, 2020

Choose a reason for hiding this comment

sindresorhus commented Feb 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus Feb 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skllcrn left a comment

Choose a reason for hiding this comment

karaggeorge commented Mar 9, 2021 • edited Loading

karaggeorge commented Mar 9, 2021 • edited Loading

sindresorhus commented Mar 10, 2021

karaggeorge commented Mar 10, 2021

sindresorhus commented Mar 10, 2021

karaggeorge commented Mar 10, 2021

sindresorhus commented Mar 10, 2021

karaggeorge commented Mar 12, 2021

sindresorhus left a comment

Choose a reason for hiding this comment

skllcrn left a comment • edited Loading

Choose a reason for hiding this comment

karaggeorge commented Mar 12, 2021 • edited Loading

sindresorhus commented Mar 13, 2021

karaggeorge commented Nov 5, 2020 •

edited

Loading

sindresorhus Feb 16, 2021 •

edited

Loading

karaggeorge commented Mar 9, 2021 •

edited

Loading

karaggeorge commented Mar 9, 2021 •

edited

Loading

skllcrn left a comment •

edited

Loading

karaggeorge commented Mar 12, 2021 •

edited

Loading