Skip to content

refactor: TypeScript rewrite#1593

Merged
erikian merged 39 commits intomainfrom
refactor/typescript-rewrite
Dec 5, 2023
Merged

refactor: TypeScript rewrite#1593
erikian merged 39 commits intomainfrom
refactor/typescript-rewrite

Conversation

@erikian
Copy link
Copy Markdown
Member

@erikian erikian commented Nov 3, 2023

  • 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:

@request-info request-info bot added the needs info Issue reporter needs to provide more information for maintainers to take action label Nov 3, 2023
@erikian erikian removed the needs info Issue reporter needs to provide more information for maintainers to take action label Nov 3, 2023
@erikian erikian force-pushed the refactor/typescript-rewrite branch from 07443bd to 456e681 Compare November 5, 2023 19:44
@electron electron deleted a comment from request-info bot Nov 5, 2023
@erikian erikian force-pushed the refactor/typescript-rewrite branch 2 times, most recently from d47c5bb to cd844b5 Compare November 5, 2023 20:40
@erikian erikian marked this pull request as ready for review November 5, 2023 20:41
@erikian erikian requested a review from a team as a code owner November 5, 2023 20:41
@erikian erikian force-pushed the refactor/typescript-rewrite branch from 74e88c6 to c3dd70d Compare November 5, 2023 20:46
@erikian erikian marked this pull request as draft November 5, 2023 20:54
@erikian erikian force-pushed the refactor/typescript-rewrite branch 3 times, most recently from c4864a2 to 2649717 Compare November 6, 2023 01:31
@codecov
Copy link
Copy Markdown

codecov bot commented Nov 6, 2023

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!

@erikian erikian force-pushed the refactor/typescript-rewrite branch 3 times, most recently from 9924874 to ffb30cc Compare November 6, 2023 04:19
@erikian erikian marked this pull request as ready for review November 6, 2023 04:27
@erikian erikian force-pushed the refactor/typescript-rewrite branch from 5b35113 to 1644eb0 Compare November 6, 2023 12:42
@@ -1,5 +1,5 @@
import { normalizePath, warning } from './common';
import galactus from 'galactus';
import { DestroyerOfModules, DepType, Module, ModuleMap } from 'galactus';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The names will always be funny to me

Copy link
Copy Markdown
Member

@felixrieseberg felixrieseberg left a comment

Choose a reason for hiding this comment

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

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.

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

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 erikian force-pushed the refactor/typescript-rewrite branch from 5ccd8d1 to 47e4006 Compare November 19, 2023 22:00
@erikian erikian force-pushed the refactor/typescript-rewrite branch from d9ada2e to 437471a Compare November 19, 2023 23:19
@erikian
Copy link
Copy Markdown
Member Author

erikian commented Nov 19, 2023

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
Copy link
Copy Markdown
Member Author

erikian commented Nov 20, 2023

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
Copy link
Copy Markdown
Member

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

@erikian erikian force-pushed the refactor/typescript-rewrite branch from 7795430 to 48a47a3 Compare December 5, 2023 01:35
@erikian erikian merged commit 3588ea1 into main Dec 5, 2023
@erikian erikian deleted the refactor/typescript-rewrite branch December 5, 2023 03:04
@continuous-auth
Copy link
Copy Markdown

🎉 This PR is included in version 18.1.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants