Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Net Fetch in Electron #3069

Merged
merged 13 commits into from
Jun 6, 2024
47 changes: 26 additions & 21 deletions .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,35 @@
"ignorePatterns": ["**/*"],
"plugins": ["@nrwl/nx"],
"overrides": [
{
Copy link
Member Author

Choose a reason for hiding this comment

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

This loosens up the eslint rules so I can @ts-ignore and use any type.

"files": ["*.ts", "*.tsx"],
"extends": ["plugin:@nrwl/nx/typescript"],
"rules": {}
},
{
"files": ["*.js", "*.jsx"],
"extends": ["plugin:@nrwl/nx/javascript"],
"rules": {}
},
{
"files": ["*.spec.ts", "*.spec.tsx", "*.spec.js", "*.spec.jsx"],
"env": {
"jest": true
},
"rules": {}
},
{
"files": ["*.ts", "*.tsx", "*.js", "*.jsx"],
"rules": {
"@typescript-eslint/no-unused-vars": ["error", {
"varsIgnorePattern": "^_",
"argsIgnorePattern": "^_"
}],
"@typescript-eslint/no-unused-vars": [
"error",
{
"varsIgnorePattern": "^_",
"argsIgnorePattern": "^_"
}
],
"@typescript-eslint/no-explicit-any": ["off", {}],
"@typescript-eslint/ban-ts-comment": ["off", {}],
"@nrwl/nx/enforce-module-boundaries": [
"error",
{
Expand All @@ -24,23 +46,6 @@
}
]
}
},
{
"files": ["*.ts", "*.tsx"],
"extends": ["plugin:@nrwl/nx/typescript"],
"rules": {}
},
{
"files": ["*.js", "*.jsx"],
"extends": ["plugin:@nrwl/nx/javascript"],
"rules": {}
},
{
"files": ["*.spec.ts", "*.spec.tsx", "*.spec.js", "*.spec.jsx"],
"env": {
"jest": true
},
"rules": {}
}
]
}
7 changes: 7 additions & 0 deletions apps/zui/src/core/electron-zed-client.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import {Client} from "@brimdata/zed-node"
import {net} from "electron"

export class ElectronZedClient extends Client {
fetch =
process.env.JEST_WORKER_ID === undefined ? net.fetch : globalThis.fetch
Copy link
Contributor

Choose a reason for hiding this comment

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

I mentioned in a previous comment that despite the app functionality and e2e tests working ok after the changes thus far, we were still seeing many failures during yarn test that looked like the following, such as in this CI run:

FAIL apps/zui/src/electron/start.test.ts
  ● app opens a window on startup

    TypeError: Cannot read properties of undefined (reading 'fetch')

      3 |
      4 | export class ElectronZedLake extends Lake {
    > 5 |   fetch = net.fetch
        |               ^
      6 | }
      7 |

      at new fetch (src/core/electron-zed-lake.ts:5:15)
      at Function.boot (src/core/main/main-object.ts:47:18)
      at Object.boot (src/electron/run-main/boot.ts:13:16)
      at Object.main (src/electron/run-main/run-main.ts:19:19)
      at Object.<anonymous> (src/electron/start.test.ts:18:20)

  ● activate creates window if there are none

Based on my understanding, these Jest tests are more lightweight so I assume the root cause is that Electron stuff like net.fetch are simply not available when Jest tests are running. I figured one way to address this would be to determine if a test was running in Jest and, if so, choose a different fetch. I did some web searches and saw others claiming success with checking Jest-ness via the presence of this JEST_WORKER_ID env var so I used that approach here and it does seem to work, as yarn test now passes locally and in CI and the e2e tests still pass. I'd appreciate some close scrutiny of my approach to confirm it's sound, though!

This comment was marked as resolved.

}
7 changes: 7 additions & 0 deletions apps/zui/src/core/electron-zed-lake.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import {Lake} from "@brimdata/zed-node"
import {net} from "electron"

export class ElectronZedLake extends Lake {
fetch =
process.env.JEST_WORKER_ID === undefined ? net.fetch : globalThis.fetch
}
9 changes: 5 additions & 4 deletions apps/zui/src/core/main/main-object.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import {app} from "electron"
import keytar from "keytar"
import {EventEmitter} from "events"
import os from "os"
import {Client, Lake} from "@brimdata/zed-node"
import {Store as ReduxStore} from "redux"
import url from "url"
import {
Expand Down Expand Up @@ -30,6 +29,8 @@ import {getAuthToken} from "../../js/api/core/get-zealot"
import {Abortables} from "src/app/core/models/abortables"
import * as zui from "src/zui"
import log from "electron-log"
import {ElectronZedClient} from "../electron-zed-client"
import {ElectronZedLake} from "../electron-zed-lake"

export class MainObject {
public isQuitting = false
Expand All @@ -43,7 +44,7 @@ export class MainObject {
const windows = new WindowManager(data)
const store = createMainStore(data?.globalState)
const appMeta = await getAppMeta()
const lake = new Lake({
const lake = new ElectronZedLake({
root: args.lakeRoot,
port: args.lakePort,
logs: args.lakeLogs,
Expand All @@ -55,7 +56,7 @@ export class MainObject {

// Only call this from boot
constructor(
readonly lake: Lake,
readonly lake: ElectronZedLake,
readonly windows: WindowManager,
readonly store: ReduxStore<State, any>,
readonly session: Session,
Expand Down Expand Up @@ -135,7 +136,7 @@ export class MainObject {
const lakeData = Lakes.id(lakeId)(this.store.getState())
const lake = createLake(lakeData)
const auth = await this.dispatch(getAuthToken(lake))
return new Client(lake.getAddress(), {auth})
return new ElectronZedClient(lake.getAddress(), {auth})
}

async createDefaultClient() {
Expand Down
3 changes: 2 additions & 1 deletion packages/zed-node/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
},
"dependencies": {
"@brimdata/zed-js": "workspace:*",
"fs-extra": "^11.1.1"
"fs-extra": "^11.1.1",
"node-fetch": "^2.6.2"
},
"peerDependencies": {
"zed": "*"
Expand Down
4 changes: 3 additions & 1 deletion packages/zed-node/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@ import {
getLoadContentType,
jsonHeader,
} from '@brimdata/zed-js';
// @ts-ignore
import nodeFetch from 'node-fetch';

export class Client extends BaseClient {
public fetch = globalThis.fetch;
public fetch = (...args: any[]) => nodeFetch(...args);

async load(
data: string | NodeJS.ReadableStream,
Expand Down
3 changes: 2 additions & 1 deletion packages/zed-node/src/lake.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ type ConstructorOpts = {
corsOrigins?: string[];
};
export class Lake {
fetch = globalThis.fetch;
lake?: ChildProcess;
root: string;
port: number;
Expand Down Expand Up @@ -82,7 +83,7 @@ export class Lake {

async isUp() {
try {
const response = await globalThis.fetch(`http://${this.addr()}/status`);
const response = await this.fetch(`http://${this.addr()}/status`);
const text = await response.text();
return text === 'ok';
} catch (e) {
Expand Down
15 changes: 15 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1922,6 +1922,7 @@ __metadata:
"@brimdata/zed-js": "workspace:*"
"@types/fs-extra": ^11.0.1
fs-extra: ^11.1.1
node-fetch: ^2.6.2
zed: "github:brimdata/zed#main"
peerDependencies:
zed: "*"
Expand Down Expand Up @@ -14347,6 +14348,20 @@ __metadata:
languageName: node
linkType: hard

"node-fetch@npm:^2.6.2":
version: 2.7.0
resolution: "node-fetch@npm:2.7.0"
dependencies:
whatwg-url: ^5.0.0
peerDependencies:
encoding: ^0.1.0
peerDependenciesMeta:
encoding:
optional: true
checksum: d76d2f5edb451a3f05b15115ec89fc6be39de37c6089f1b6368df03b91e1633fd379a7e01b7ab05089a25034b2023d959b47e59759cb38d88341b2459e89d6e5
languageName: node
linkType: hard

"node-gyp-build@npm:^4.3.0":
version: 4.6.0
resolution: "node-gyp-build@npm:4.6.0"
Expand Down
Loading