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
Feat: Converted the main EP to typescript. #11
Feat: Converted the main EP to typescript. #11
Conversation
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.
Thanks for this, this is a good change. Need to address a few things before merging this in. Also, make sure you run the lint
and format
scripts after you're done.
}) | ||
|
||
/** @param {unknown} error */ | ||
function getErrorStack(error: any) { |
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.
function getErrorStack(error: any) { | |
function getErrorStack(error: unknown) { |
"clean": "del-cli dist desktop/build public/build .cache", | ||
"dev": "npm run clean && cross-env NODE_ENV=development npm-run-all --parallel --print-label --race dev:*", | ||
"clean": "del-cli dist desktop/build public/build .cache desktop/main.js", | ||
"dev": "npm run clean && npm run build:desktop && cross-env NODE_ENV=development npm-run-all --parallel --print-label --race dev:*", |
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.
Correct me if I'm wrong, but this appears to only build the entry once before running dev. Would prefer if it watched the entry file and rebuilt on changes instead
"dev:remix": "remix watch", | ||
"dev:nodemon": "wait-on file:desktop/main.js && nodemon .", | ||
"build": "npm run clean && remix build && electron-builder", | ||
"build": "npm run clean && npm run build:desktop && remix build && electron-builder", | ||
"build:desktop": "yarn tsc -p tsconfig.electron-build.json", |
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.
- Let's use
npm
instead ofyarn
. It's much more universal - I think tsup-node would make a good, faster replacement for
tsc
here. Then you can remove thetsconfig.electron-build.json
file.
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.
Also noticed a few dependencies could use some updating
- we're on typescript 4.5.5, current the latest stable typescript version is
4.6.2
Other dependencies in root package.json file, which need small version bumps.
What do you think about upgrading this in this PR?
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.
Sure, fine by me
The reason the electron peer is at >=16 is because the library should still work fine with it, esp. for those who can't use 17 for whatever reason. Generally speaking, I believe the wider the peers are, the better. Less friction
"desktop/main.ts" | ||
], | ||
"exclude": ["desktop/build", "public/build", ".cache"] | ||
} |
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.
Hi @itsMapleLeaf @itsMapleLeaf
Given that electron bundles a full version of modern version of chrome
- electron 17.1.0 the version we're using bundles chromium version 98 (my modern chrome browser that is non electron is chromium 99)
I believe its safe to make the following suggest changes below changes, which would significantly reduce the bundle size & speed up app execution
- change
target
to ->target: esnext
- change
lib
to:lib: ["DOM", "DOM.Iterable", "esnext"]
- this way we get typescript compiler to include modern types for the browser environment and node
- docs: https://www.typescriptlang.org/tsconfig#lib
Same suggestion for the template/tsconfig.json
file here:
What do you think?
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.
I think the rationale is a good one, and I'm pretty much always in favor of targeting modern JS, even outside of "stable" environments like Electron. But here, I suggested to use tsup instead, which uses esbuild under the hood, which should accomplish the same goal, and changing the TSConfig target doesn't affect the emit.
Changing the lib is fine though
@ITninja04 the README.md file inside of the template file could use some minor updates for first time users😄 |
These are good changes, and I'll probably incorporate those outside of this pr, thanks 🙂 Although I don't recommend using yarn for public documentation, as less people are familiar with it, + yarn/pnpm users will likely be aware of the analogs from npm. |
Extended the base tsconfig to cover the new main.ts entry point. Added a new build:desktop script to build the main.ts entry point to JS. Added the aforementioned script to the dev and build scripts so the file is rebuilt before the app is launched.
da2f588
to
973bc96
Compare
Extended the base tsconfig to cover the new main.ts entry point.
Added a new build:desktop script to build the main.ts entry point to JS.
Added the aforementioned script to the dev and build scripts so the file is rebuilt before the app is launched.