feat: app lazy-loads app-context #2378

SgtPooki · 2023-01-17T20:59:42Z

This PR is focused on updating ctx so that it can tolerate extension, updating, and no longer requires specific imports or tribal knowledge to use.

A side-effect of this PR is that the latency of app startup is reduced significantly.

chore: update ctx refactor
fix: tests
feat: add context.spec.js tests
fix: startup issues
feat: app context supports lazy functions

fixes #1177

Benchmarks

Note: nothing else was actively running on my laptop when the "statistical outliers" were detected. It's just that doing things in parallel reduces the chances for outliers to occur because we're doing everything as fast as we can.

> hyperfine --parameter-list branch main,feat/ctx-refactor --setup "git switch {branch}" "npm run test:e2e"
Benchmark 1: npm run test:e2e (branch = main)
  Time (mean ± σ):     26.950 s ±  0.610 s    [User: 13.728 s, System: 3.717 s]
  Range (min … max):   26.491 s … 28.485 s    10 runs

  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

Benchmark 2: npm run test:e2e (branch = feat/ctx-refactor)
  Time (mean ± σ):     22.183 s ±  0.229 s    [User: 11.185 s, System: 3.115 s]
  Range (min … max):   21.973 s … 22.783 s    10 runs

Summary
  npm run test:e2e (branch = feat/ctx-refactor) ran
    1.21 ± 0.03 times faster than npm run test:e2e (branch = main)

hyperfine --parameter-list branch main,feat/ctx-refactor --setup "git switch {branch}" --runs 25 -w 1 "npm run test:e2e"
Benchmark 1: npm run test:e2e (branch = main)
  Time (mean ± σ):     27.262 s ±  0.798 s    [User: 13.872 s, System: 3.751 s]
  Range (min … max):   26.923 s … 31.025 s    25 runs

  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

Benchmark 2: npm run test:e2e (branch = feat/ctx-refactor)
  Time (mean ± σ):     22.785 s ±  0.331 s    [User: 11.432 s, System: 3.327 s]
  Range (min … max):   22.396 s … 23.913 s    25 runs

Summary
  npm run test:e2e (branch = feat/ctx-refactor) ran
    1.20 ± 0.04 times faster than npm run test:e2e (branch = main)

SgtPooki

these comments are on code I pushed prior to this PR self review..

src/daemon/index.js

SgtPooki · 2023-01-17T21:04:56Z

src/handleError.js

@@ -0,0 +1,25 @@
+// @ts-check


just abstracting this out from src/index.js

SgtPooki · 2023-01-17T21:05:24Z

src/i18n.js

  await i18n
+    // @ts-expect-error


addressing some type errors

SgtPooki · 2023-01-17T21:06:03Z

src/index.js

+  if (process.env.HOME) {
+    app.setPath('home', process.env.HOME)
+    app.setPath('userData', path.join(process.env.HOME, 'data'))
+  }


addressing type error (cannot set path to string|undefined)

src/index.js

SgtPooki · 2023-01-17T21:25:04Z

tsconfig.json

@@ -47,7 +47,7 @@
    "emitDeclarationOnly": true,                      /* Only output d.ts files and not JavaScript files. */
    // "sourceMap": true,                                /* Create source map files for emitted JavaScript files. */
    // "outFile": "./",                                  /* Specify a file that bundles all outputs into one JavaScript file. If `declaration` is true, also designates a file that bundles all .d.ts output. */
-    // "outDir": "./",                                   /* Specify an output folder for all emitted files. */
+    "outDir": "./dist",                                   /* Specify an output folder for all emitted files. */


ran npx -y tsc and it output a ton of files into src/ that had me scratching my head for a while. I had updated code, but types were still suck to what they were when I ran tsc. 🙄

src/common/config-keys.js

SgtPooki · 2023-01-17T21:28:03Z

src/common/logger.js

@@ -57,7 +58,7 @@ logger.info(`[meta] logs can be found on ${logsPath}`)

 /**
 *
- * @param {AnalyticsTimeOptions & import('countly-sdk-nodejs').CountlyAddEventOptions}
+ * @param {AnalyticsTimeOptions & import('countly-sdk-nodejs').CountlyAddEventOptions} args


fixing error for unnamed parameter

src/context.js

SgtPooki · 2023-01-17T21:29:40Z

src/context.js

+      // @ts-ignore
+      return value


typing is fairly loose in this right now, but resolving that would require updating the entire codebase

SgtPooki · 2023-07-08T01:33:51Z

e2e tests are passing for me locally now. idk whats going on here.

SgtPooki · 2023-07-08T01:56:45Z

tried reverting the daemonReady function.. its passing for me locally again.. lets see if that works on CI

whizzzkid

Took me a while to grasp this, do we need a testing strategy around this? Added a few questions/concerns.

One quick nit (this is not really better in the current release), once ipfs-desktop boots, I have to double click to switch between tabs.

2387-dbl-clk.mov

src/context.js

whizzzkid · 2023-07-08T08:34:44Z

src/context.js

+    const originalFnPromise = this.getProp(propertyName)
+
+    return async (...args) => {
+      const originalFn = await originalFnPromise


uhm, why have a closure here?

To delay the await until the function is actually called. If we await before it's called then we end up blocking before we need to.

src/context.js

whizzzkid · 2023-07-08T08:36:15Z

src/context.js

+      this._createDeferredForProp(propertyName)
+      this._resolvePropForValue(propertyName, value)


why is this needed?

src/context.js

whizzzkid · 2023-07-08T08:41:44Z

src/context.js

overall I think this is abstracting a lot of complex logic and creating a promise(-hell-ish) landscape, I wonder if there's a way to simplify this.

I hope you feel differently now. This is simplifying things drastically because all you do is set/get properties that may be set/gotten elsewhere. This unblocks a number of things we couldn't do previously, and doesn't require a complete re-write. see #1177 for more information

Also, please double-check test/unit/context.spec.js to confirm that we have enough tests to ensure the core is functioning how we expect.

src/context.js

src/handleError.js

src/tray.js

SgtPooki · 2023-07-10T02:50:17Z

A logic table for the context class:

Context property exists?	Is the backing promise fulfilled?	Method called	Is a deferred promise created?	Returned Value
No	N/A	GetProp	Yes	A newly created deferred promise(unfulfilled)
No	N/A	SetProp	Yes	A newly created deferred promise(fulfilled)
Yes	No	GetProp	No	The found deferred promise (unfulfilled)
Yes	No	SetProp	No	The found deferred promise (fulfilled)
Yes	Yes	GetProp	No	The found deferred promise (fulfilled)
Yes	Yes	SetProp	No	We should throw/log an error here. Any getProps called for the property prior to this will have a hanging promise.

SgtPooki · 2023-07-10T03:29:43Z

One quick nit (this is not really better in the current release), once ipfs-desktop boots, I have to double click to switch between tabs.

This is an existing bug: #762

SgtPooki

another self review

SgtPooki · 2023-07-18T18:04:02Z

src/context.js

I hope you feel differently now. This is simplifying things drastically because all you do is set/get properties that may be set/gotten elsewhere. This unblocks a number of things we couldn't do previously, and doesn't require a complete re-write. see #1177 for more information

SgtPooki · 2023-07-18T18:06:58Z

src/context.js

Also, please double-check test/unit/context.spec.js to confirm that we have enough tests to ensure the core is functioning how we expect.

src/tray.js

SgtPooki · 2023-07-18T18:18:44Z

test/unit/context.spec.js

+  test('can get and call a function before its set', async () => {
+    let isPending = true
+    const spy = sinon.spy()
+    const actualLazyFn = (...args) => {
+      isPending = false
+      spy(...args)
+    }
+    const lazyFn = ctx.getFn('lazyFn')
+    const lazyFnCallPromise = lazyFn(123)
+    expect(isPending).toBe(true)
+    expect(spy.callCount).toBe(0)
+    expect(ctx.getProp('lazyFn')).resolves.toBe(actualLazyFn)
+    ctx.setProp('lazyFn', actualLazyFn)
+    await lazyFnCallPromise
+    expect(isPending).toBe(false)
+    expect(spy.callCount).toBe(1)
+    expect(spy.getCall(0).args).toEqual([123])
+  })


@whizzzkid this test confirms the lazy function execution works like we expect.

const fn = ctx.getFn('someFnThatMayBeSet_orNot') await fn() // can be called at any time.. regardless of when we actually set the value.

SgtPooki · 2023-07-18T18:19:08Z

test/unit/add-to-ipfs.spec.js

+    const cid = await addToIpfs(getFixtures('dir'))
    expect(electron.clipboard.writeText.callCount).toEqual(1)
    expect(notify.notifyError.callCount).toEqual(0)
    expect(notify.notify.callCount).toEqual(1)
    expect(cid.toString()).toEqual('bafybeieyzpi3qdqtj7b7hfzdpt2mnmaiicvtrj7ngmf2fc76byecj62gea')
  })

  test('add to ipfs multiple files', async () => {
-    const cid = await addToIpfs(ctx, getFixtures('dir', 'hello-world.txt'))
+    const cid = await addToIpfs(getFixtures('dir', 'hello-world.txt'))


we no longer need to pass ctx around.

SgtPooki · 2023-07-18T18:21:35Z

src/webui/preload.js

+const apiAddress = urlParams.get('api')
+if (apiAddress != null) {
+  window.localStorage.setItem('ipfsApi', apiAddress)
+}


fixes console error spotted when testing

whizzzkid

lgtm

SgtPooki added 5 commits January 16, 2023 23:44

chore: update ctx refactor

7c02e34

fix: tests

b947d61

feat: add context.spec.js tests

042f858

fix: startup issues

472219c

feat: app context supports lazy functions

93a2bac

SgtPooki requested a review from a team as a code owner January 17, 2023 20:59

This was referenced Jan 17, 2023

fix: Refactor ctx #2114

Closed

1177 refactor ctx speed improvements #2125

Closed

chore: self-PR-review cleanup

94e752e

SgtPooki commented Jan 18, 2023

View reviewed changes

tinytb mentioned this pull request Jan 18, 2023

Refactor ctx #1177

Closed

SgtPooki marked this pull request as draft May 4, 2023 17:46

SgtPooki added 3 commits May 4, 2023 10:46

Merge branch 'main' into feat/ctx-refactor

b6124fc

Merge branch 'main' into feat/ctx-refactor

221612d

test: fix e2e tests

07ff889

SgtPooki mentioned this pull request Jul 8, 2023

feat: add splash screen on top of ctx refactor #2548

Merged

SgtPooki added 2 commits July 7, 2023 18:55

chore(deps): upgrade playwright

118825c

fix: revert daemonReady function

c160aa0

SgtPooki marked this pull request as ready for review July 8, 2023 02:16

SgtPooki requested a review from whizzzkid as a code owner July 8, 2023 02:16

whizzzkid approved these changes Jul 8, 2023

View reviewed changes

SgtPooki added 3 commits July 18, 2023 10:21

fix: update tray menu is ctx function

79ef984

feat: App context improvements

103ce56

Merge branch 'main' into feat/ctx-refactor

8971129

SgtPooki commented Jul 18, 2023

View reviewed changes

chore: minor cleanup and comments

3ac5c88

chore: debug logs to address PR comment

593be2c

SgtPooki self-assigned this Jul 18, 2023

whizzzkid approved these changes Jul 18, 2023

View reviewed changes

SgtPooki merged commit 4641dcb into main Jul 18, 2023

SgtPooki deleted the feat/ctx-refactor branch July 18, 2023 22:51

SgtPooki mentioned this pull request Aug 29, 2023

fix: tray translation waits for i18n init #2604

Merged

whizzzkid mentioned this pull request Sep 18, 2023

Taskbar icon not working #2627

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: app lazy-loads app-context #2378

feat: app lazy-loads app-context #2378

SgtPooki commented Jan 17, 2023 •

edited

Loading

SgtPooki left a comment

SgtPooki Jan 17, 2023

SgtPooki Jan 17, 2023

SgtPooki Jan 17, 2023

SgtPooki Jan 17, 2023

SgtPooki Jan 17, 2023

SgtPooki Jan 17, 2023

SgtPooki commented Jul 8, 2023

SgtPooki commented Jul 8, 2023

whizzzkid left a comment

whizzzkid Jul 8, 2023

SgtPooki Jul 18, 2023 •

edited

Loading

whizzzkid Jul 8, 2023

whizzzkid Jul 8, 2023

SgtPooki Jul 18, 2023

SgtPooki Jul 18, 2023

SgtPooki commented Jul 10, 2023 •

edited

Loading

SgtPooki commented Jul 10, 2023

SgtPooki left a comment

SgtPooki Jul 18, 2023

SgtPooki Jul 18, 2023

SgtPooki Jul 18, 2023

SgtPooki Jul 18, 2023

SgtPooki Jul 18, 2023

whizzzkid left a comment

		this._createDeferredForProp(propertyName)
		this._resolvePropForValue(propertyName, value)

feat: app lazy-loads app-context #2378

feat: app lazy-loads app-context #2378

Conversation

SgtPooki commented Jan 17, 2023 • edited Loading

Benchmarks

SgtPooki left a 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

SgtPooki commented Jul 8, 2023

SgtPooki commented Jul 8, 2023

whizzzkid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SgtPooki Jul 18, 2023 • 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

Choose a reason for hiding this comment

SgtPooki commented Jul 10, 2023 • edited Loading

SgtPooki commented Jul 10, 2023

SgtPooki left a 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

whizzzkid left a comment

Choose a reason for hiding this comment

SgtPooki commented Jan 17, 2023 •

edited

Loading

SgtPooki Jul 18, 2023 •

edited

Loading

SgtPooki commented Jul 10, 2023 •

edited

Loading