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

Core electron + core backend ESM #7293

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Core electron + core backend ESM #7293

wants to merge 7 commits into from

Conversation

hl662
Copy link
Contributor

@hl662 hl662 commented Oct 24, 2024

Notables:

  • Add dual cjs/esm builds for core backend and electron
  • Electron needs min range bumped from 23 to 28 BREAKING
  • (WIP) Exploring converting DTA's backend code to be ESM, and consume esm version of backend and electron.

Note: tsc does not add .js extensions to extensionless imports (unlike Vite :( ), so had to add .js at the end of our relative import paths in DTA's backend.

@@ -39,7 +52,7 @@
"@itwin/core-bentley": "workspace:^4.10.0-dev.30",
"@itwin/core-common": "workspace:^4.10.0-dev.30",
"@itwin/core-frontend": "workspace:^4.10.0-dev.30",
"electron": ">=23.0.0 <34.0.0"
"electron": ">=28.0.0 <34.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

what happens if we leave the min ver as is?

Copy link
Contributor

Choose a reason for hiding this comment

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

Electron "properly" supports ESM from version 28. If we're delivering both CJS and ESM, we could leave version 23 as minimum, I think.

"build:cjs": "tsc 1>&2 --outDir lib/cjs",
"build:esm": "tsc 1>&2 --module ES2020 --outDir lib/esm",
Copy link
Member

Choose a reason for hiding this comment

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

yourec ompiling core-electron to module esnext, but here es2020?

@@ -5,7 +5,7 @@
import { IModelRpcProps, RpcInterface, RpcManager, TextAnnotationProps, TextBlockGeometryProps } from "@itwin/core-common";
import * as http from "http";
Copy link
Member

Choose a reason for hiding this comment

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

what are we using "backendServer" for?

Copy link
Member

Choose a reason for hiding this comment

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

this shouldnt be in the common layer imo

"build:cjs": "tsc 1>&2 --outDir lib/cjs",
"build:esm": "tsc 1>&2 --module esnext --outDir lib/esm",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why deliver both CJS and ESM ? Wouldn't it be less painful to just switch to ESM on 5.0 ?

Copy link
Contributor Author

@hl662 hl662 Oct 28, 2024

Choose a reason for hiding this comment

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

yeah, I was initially thinking this would go into one of the 4.X versions, but I'm now more certain this should be in 5.x - as I go through with modifying DTA, I'm noticing pain points with delivering dual cjs/esm, both in these packages and our electron-authorization pkg. When running an ESM module, and using named imports (like "@itwin/electron-authorization/Main"), electron complains at run time that it doesn't like electron-auth being CJS. And seems like the only way to solve that is to set 'type': "module" in the respective package.json.

@hl662
Copy link
Contributor Author

hl662 commented Oct 30, 2024

Pausing work on this PR for now, but documenting the work so far:

  • Moved display-test-app to all ESM, but having runtime issues.
  • When moving core-electron to use export maps, there's one important thing to note: Any ESM-based application that uses a dependency's export maps requires that dependency to be ESM (which is to have "type": "module" in it's package.json). This is relevant to @itwin/electron-authorization, because we have export maps in there. I experimented with making that package to be ESM, pushing my work to a remote branch. I confirmed that electron-authorization's tokenStore was able to run, successfully accessing my OS-based credentials and utilizing 'electron-store' under the hood, so the branch I linked above demonstrates the feasibility of publishing a ESM-based NPM package for backends.
  • I haven't dug into the __dirname is not defined error, but it pops up on the frontend side of the electron app, suggesting that our vite.config.js needs to be updated to use electron's ESM version, rather than the CJS version. I suspect that if we are able to solve this particular issue, we'll see more than just the spinner on startup
  • I haven't dug into why the preload script is not being able to be loaded - everything does point to the backend being able to find ElectronPreload.mjs though (there was a prior runtime error saying it couldn't find the file). Maybe @GytisCepk could have a better idea on what is going on...
  • To reproduce what I see in DTA, go to core/electron/ and run just pnpm build:esm - I'm using the import.meta.url in ElectronHost.ts, so building CJS will throw build errors. Then go to test-apps/display-test-app and run pnpm build followed by pnpm start.
  • Have not seen evidence otherwise that we can build dual CJS/ESM for core-electron, solely because of import.meta.url. CJS allows require but ESM doesn't. ESM allows import.meta.url but CJS doesn't. Why is this important? core-electron's ElectronHost requires a path to ElectronPreload.mjs passed into one of it's config, and the 2 ways above are the 2 unique and correct ways to resolve file paths within a module.

Image of the runtime issue:
image

@GytisCepk
Copy link
Contributor

I haven't dug into why the preload script is not being able to be loaded - everything does point to the backend being able to find ElectronPreload.mjs though (there was a prior runtime error saying it couldn't find the file).

ESM is not supported for preload script if sandbox is enabled (Electron ESM support matrix). If we plan to drop CJS from both backend and frontend code for Electron, we'll still need to transpile preload script to CJS.

@GytisCepk
Copy link
Contributor

I haven't dug into the __dirname is not defined error, but it pops up on the frontend side of the electron app, suggesting that our vite.config.js needs to be updated to use electron's ESM version, rather than the CJS version.

Isn't __dirname only available in CJS ? __dirname Node.js docs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants