refactor: TypeScript rewrite #1593

erikian · 2023-11-03T22:20:29Z

I have read the contribution documentation for this project.
I agree to follow the code of conduct that this project follows, as appropriate.
The changes are appropriately documented (if applicable).
The changes have sufficient test coverage (if applicable).
The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:

codecov · 2023-11-06T01:37:27Z

Codecov Report

Merging #1593 (5ccd8d1) into main (4e3ab8a) will decrease coverage by 11.24%.
The diff coverage is 84.42%.

@@             Coverage Diff             @@
##             main    #1593       +/-   ##
===========================================
- Coverage   95.65%   84.42%   -11.24%     
===========================================
  Files          15       16        +1     
  Lines         783      828       +45     
  Branches        0      163      +163     
===========================================
- Hits          749      699       -50     
- Misses         34       94       +60     
- Partials        0       35       +35

Files	Coverage Δ
src/hooks.ts	`100.00% <100.00%> (ø)`
src/index.ts	`100.00% <100.00%> (ø)`
src/linux.ts	`100.00% <100.00%> (ø)`
src/prune.ts	`100.00% <100.00%> (ø)`
src/unzip.ts	`100.00% <100.00%> (ø)`
src/common.ts	`98.43% <98.43%> (ø)`
src/download.ts	`94.73% <94.73%> (ø)`
src/infer.ts	`98.68% <98.68%> (ø)`
src/cli.ts	`95.34% <95.34%> (ø)`
src/win32.ts	`95.55% <95.55%> (ø)`
... and 6 more

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

felixrieseberg · 2023-11-06T18:13:35Z

src/prune.ts

@@ -1,5 +1,5 @@
 import { normalizePath, warning } from './common';
-import galactus from 'galactus';
+import { DestroyerOfModules, DepType, Module, ModuleMap } from 'galactus';


The names will always be funny to me

src/hooks.ts

felixrieseberg

I gave this whole PR one solid read - and even though it's massive and I think I should have found one place to provide constructive feedback, this seems mostly pretty good to me.

The one thing we should make double-and-triple sure before merging is that nobody can actually rely on electron-packager/hooks - and if they could, that we maybe just keep it and cut it in a later PR.

src/hooks.ts

src/common.ts

.circleci/config.yml

.gitignore

package.json

malept · 2023-11-07T06:12:23Z

package.json

-  "main": "src/index.js",
-  "types": "src/index.d.ts",
+  "main": "dist/index.js",
+  "types": "dist/types.d.ts",
  "bin": {
    "electron-packager": "bin/electron-packager.js"


issue: I'm pretty sure you'll need to update this file. (Although I don't know if I'd necessarily convert it to TypeScript, since it's basically a Node.js-version-checking shim for src/cli.)

We're still shipping the bin directory to npm, so the path here is fine — I did forgot to update a require in that file from src to dist though 😅

That being said, do we really need this file? Like, if the user has e.g. Node 10 and we require Node 16, the package manager should prevent packager from being installed (unless they're using --ignore-engines, but then the user should be aware that things can break)

You'd be surprised how many issues were filed back in the day about why there were SyntaxErrors even though the minimum engine has been specified for a very long time.

I happened to find a relatively new instance of this with Electron Forge in the Discord server: the user was using Node.js 12 when Forge requires Node.js 16, and Forge definitely has engine constraints in its various package.json files.

Yeah, now I see why the validation is there, we shouldn't trust engines at all. Adding a proper validation to the ~40 packages in the Forge monorepo is gonna be fun 😟

I would just add it to the two CLI-facing packages: create-electron-app and @electron-forge/cli.

.eslintrc.js

dsanders11 · 2023-11-08T01:43:21Z

Something to watch out for, there's at least one usage in Forge that directly reaches into src/: https://github.com/electron/forge/blob/fd00d9abf1dfc6b34deadfea62ea72d4589105c8/packages/api/core/src/util/parse-archs.ts#L5

erikian · 2023-11-19T23:39:55Z

Something to watch out for, there's at least one usage in Forge that directly reaches into src/: https://github.com/electron/forge/blob/fd00d9abf1dfc6b34deadfea62ea72d4589105c8/packages/api/core/src/util/parse-archs.ts#L5

early draft so I don't forget it: electron/forge@c9da714

erikian · 2023-11-20T01:15:17Z

My n=1 experiment says removing this hack, originally introduced in #381, doesn't result in a catastrophic failure. The PR only mentions some truncated text when displaying the usage info, so we should be good 🤞🏼

dsanders11 · 2023-11-28T22:43:28Z

@erikian, couple of conflicts due to other PRs landing.

…ewrite # Conflicts: # package.json # yarn.lock

…ewrite

…ct resolution

continuous-auth · 2023-12-18T20:36:56Z

🎉 This PR is included in version 18.1.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

request-info bot added the needs info Issue reporter needs to provide more information for maintainers to take action label Nov 3, 2023

erikian removed the needs info Issue reporter needs to provide more information for maintainers to take action label Nov 3, 2023

erikian force-pushed the refactor/typescript-rewrite branch from 07443bd to 456e681 Compare November 5, 2023 19:44

electron deleted a comment from request-info bot Nov 5, 2023

erikian force-pushed the refactor/typescript-rewrite branch 2 times, most recently from d47c5bb to cd844b5 Compare November 5, 2023 20:40

erikian marked this pull request as ready for review November 5, 2023 20:41

erikian requested a review from a team as a code owner November 5, 2023 20:41

erikian force-pushed the refactor/typescript-rewrite branch from 74e88c6 to c3dd70d Compare November 5, 2023 20:46

erikian marked this pull request as draft November 5, 2023 20:54

erikian force-pushed the refactor/typescript-rewrite branch 3 times, most recently from c4864a2 to 2649717 Compare November 6, 2023 01:31

erikian force-pushed the refactor/typescript-rewrite branch 3 times, most recently from 9924874 to ffb30cc Compare November 6, 2023 04:19

erikian marked this pull request as ready for review November 6, 2023 04:27

erikian force-pushed the refactor/typescript-rewrite branch from 5b35113 to 1644eb0 Compare November 6, 2023 12:42

felixrieseberg reviewed Nov 6, 2023

View reviewed changes

src/hooks.ts Show resolved Hide resolved

felixrieseberg approved these changes Nov 6, 2023

View reviewed changes

This was referenced Nov 7, 2023

chore: bump tsd from 0.14.0 to 0.29.0 #1556

Closed

chore: bump typescript from 4.8.4 to 5.2.2 #1554

Closed

malept reviewed Nov 7, 2023

View reviewed changes

erikian force-pushed the refactor/typescript-rewrite branch from 83bf2ce to 5ccd8d1 Compare November 7, 2023 20:39

erikian added 3 commits November 19, 2023 17:26

chore: update dependencies and install missing types

be461ea

refactor: move ESLint config and ignore rules out of package.json

534afb5

refactor: rename everything to .ts

9276592

erikian added 6 commits November 19, 2023 17:26

test: update import paths

1c65c93

chore: remove tsd

6a14f56

chore: update lint script

7ae0ddc

build: update scripts

6d9ddf3

chore: update per PR feedback

946cd29

chore: re-enable and document the serialHooks function

47e4006

erikian force-pushed the refactor/typescript-rewrite branch from 5ccd8d1 to 47e4006 Compare November 19, 2023 22:00

erikian added 2 commits November 19, 2023 19:07

chore: make allOfficialArchsForPlatformAndVersion a public API

f779f0b

test: fix macOS tests

437471a

erikian force-pushed the refactor/typescript-rewrite branch from d9ada2e to 437471a Compare November 19, 2023 23:19

erikian mentioned this pull request Nov 19, 2023

chore: update Packager electron/forge#3419

Merged

5 tasks

erikian added 2 commits November 19, 2023 21:13

chore: remove deprecatedParameter

32af02c

chore: remove old hack to see if anything explodes

b19a813

erikian added 3 commits November 28, 2023 20:07

Merge remote-tracking branch 'origin/main' into refactor/typescript-r…

707e923

…ewrite # Conflicts: # package.json # yarn.lock

Merge remote-tracking branch 'origin/main' into refactor/typescript-r…

27058d4

…ewrite

chore: re-add and type @electron/windows-sign features after confli…

48a47a3

…ct resolution

erikian force-pushed the refactor/typescript-rewrite branch from 7795430 to 48a47a3 Compare December 5, 2023 01:35

erikian added 2 commits December 4, 2023 23:43

chore: fix errors from previous conflict resolutions

354701c

chore: fix types in package.json

60aab32

erikian merged commit 3588ea1 into main Dec 5, 2023

erikian deleted the refactor/typescript-rewrite branch December 5, 2023 03:04

dsanders11 mentioned this pull request Dec 13, 2023

chore: update minimum Node.js version to 16.13 #1630

Merged

2 tasks

continuous-auth bot added the released label Dec 18, 2023

This was referenced Dec 19, 2023

Electron Packager 18.1.2 breaks electron-forge #1637

Closed

Electron Packager 18.1.2 breaks electron-forge electron/forge#3443

Closed

erikian mentioned this pull request Jun 26, 2024

chore: replace legacy var with const #1742

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: TypeScript rewrite #1593

refactor: TypeScript rewrite #1593

erikian commented Nov 3, 2023 •

edited

Loading

codecov bot commented Nov 6, 2023 •

edited

Loading

felixrieseberg Nov 6, 2023

felixrieseberg left a comment

malept Nov 7, 2023

erikian Nov 7, 2023

malept Nov 7, 2023

malept Nov 19, 2023

erikian Nov 19, 2023

malept Nov 19, 2023

dsanders11 commented Nov 8, 2023

erikian commented Nov 19, 2023

erikian commented Nov 20, 2023

dsanders11 commented Nov 28, 2023

continuous-auth bot commented Dec 18, 2023

refactor: TypeScript rewrite #1593

refactor: TypeScript rewrite #1593

Conversation

erikian commented Nov 3, 2023 • edited Loading

codecov bot commented Nov 6, 2023 • edited Loading

Codecov Report

felixrieseberg Nov 6, 2023

Choose a reason for hiding this comment

felixrieseberg left a comment

Choose a reason for hiding this comment

malept Nov 7, 2023

Choose a reason for hiding this comment

erikian Nov 7, 2023

Choose a reason for hiding this comment

malept Nov 7, 2023

Choose a reason for hiding this comment

malept Nov 19, 2023

Choose a reason for hiding this comment

erikian Nov 19, 2023

Choose a reason for hiding this comment

malept Nov 19, 2023

Choose a reason for hiding this comment

dsanders11 commented Nov 8, 2023

erikian commented Nov 19, 2023

erikian commented Nov 20, 2023

dsanders11 commented Nov 28, 2023

continuous-auth bot commented Dec 18, 2023

erikian commented Nov 3, 2023 •

edited

Loading

codecov bot commented Nov 6, 2023 •

edited

Loading