-
Notifications
You must be signed in to change notification settings - Fork 210
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
base: master
Are you sure you want to change the base?
Changes from 5 commits
ae4bf9c
5d44b49
d374085
c6f4849
38e9a0d
1524b03
0a6f225
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,9 +6,22 @@ | |
"engines": { | ||
"node": "^18.0.0 || ^20.0.0" | ||
}, | ||
"exports": { | ||
"./ElectronBackend": { | ||
"types": "./lib/esm/ElectronBackend.d.ts", | ||
"import": "./lib/esm/ElectronBackend.js", | ||
"require": "./lib/cjs/ElectronBackend.js" | ||
}, | ||
"./ElectronFrontend": { | ||
"types": "./lib/esm/ElectronFrontend.d.ts", | ||
"import": "./lib/esm/ElectronFrontend.js", | ||
"require": "./lib/cjs/ElectronFrontend.js" | ||
} | ||
}, | ||
"scripts": { | ||
"build": "npm run -s build:cjs && npm run -s webpack:test", | ||
"build": "npm run -s build:cjs && npm run -s build:esm && npm run -s webpack:test", | ||
"build:cjs": "tsc 1>&2 --outDir lib/cjs", | ||
"build:esm": "tsc 1>&2 --module esnext --outDir lib/esm", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
"clean": "rimraf lib .rush/temp/package-deps*.json", | ||
"docs": "betools docs --includes=../../generated-docs/extract --json=../../generated-docs/core/core-electron/file.json --tsIndexFile=./__DOC_ONLY__.ts --onlyJson", | ||
"extract-api": "betools extract-api --entry=__DOC_ONLY__", | ||
|
@@ -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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what happens if we leave the min ver as is? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
}, | ||
"devDependencies": { | ||
"@itwin/build-tools": "workspace:*", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ | |
import { IModelRpcProps, RpcInterface, RpcManager, TextAnnotationProps, TextBlockGeometryProps } from "@itwin/core-common"; | ||
import * as http from "http"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what are we using "backendServer" for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this shouldnt be in the common layer imo |
||
import * as https from "https"; | ||
import { DtaConfiguration } from "./DtaConfiguration"; | ||
import { DtaConfiguration } from "./DtaConfiguration.js"; | ||
|
||
/** Display Test App RPC interface. */ | ||
export class DtaRpcInterface extends RpcInterface { // eslint-disable-line deprecation/deprecation | ||
|
There was a problem hiding this comment.
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?