Skip to content

refactor: improve Options types#1830

Merged
erickzhao merged 5 commits into
mainfrom
better-types
Sep 18, 2025
Merged

refactor: improve Options types#1830
erickzhao merged 5 commits into
mainfrom
better-types

Conversation

@erickzhao

@erickzhao erickzhao commented Sep 17, 2025

Copy link
Copy Markdown
Member

This PR attempts to simplify how we handle the Options interface in Electron Packager.

There are two main problems to address:

  1. External: the TargetArch and TargetPlatform types used in the Options interface are aliases to string. This is because we accept string values for custom mirrors of Electron, so any string const value passed in gets swallowed by the resulting type.
// the following types are functionally equivalent in TS!
type TargetArch1 = 'arch1' | 'arch2' | string;
type TargetArch2 = string;
  1. Internal: the Options interface defines what options can be passed in as values, but also serves as the interface that we use to pass to the final packaging function. Along the way, many optional properties in Options are inferred and mutated. This approach makes our types inaccurate internally because we're using Options everywhere, even after inference occurs and some optional properties become required.

At a high level, we're addressing the two problems with some type refactors.

  1. We no longer support arbitrary string values as platform and arch inputs in the types. This removes the superfluous ArchOption, PlatformOption, TargetArch, and TargetPlatform types. Users passing custom arch/platform values can still do so, but must cast their parameters using the OfficalArch and OfficialPlatform types.
  2. Options now explicitly refers to the options that are passed in by the user. ProcessedOptions refers to the Options object after it is processed/inferred via metadata. ProcessedOptionsWithSinglePlatformArch refers to the ProcessedOptions object once it is guaranteed to have a single platform or arch value (rather than an array or comma-separated string). We use a fresh opts object every time the shape of our options changes rather than mutating the initial Options being passed in.

Note

There are some additional refactors we can make for future work to make this delineation clearer.
For example, the Packager class takes in ProcessedOptions as a parameter, which means opts.arch and opts.platform can be an array. However, these values are never used—Packager instance functions are only ever called with a separate ProcessedOptionsWithSinglePlatformArch arg.

@request-info

request-info Bot commented Sep 17, 2025

Copy link
Copy Markdown

Thanks for filing this issue/PR! It would be much appreciated if you could provide us with more information so we can effectively analyze the situation in context.

@request-info request-info Bot added the needs info Issue reporter needs to provide more information for maintainers to take action label Sep 17, 2025
@erickzhao erickzhao removed the needs info Issue reporter needs to provide more information for maintainers to take action label Sep 17, 2025
@erickzhao erickzhao marked this pull request as ready for review September 17, 2025 22:05
@erickzhao erickzhao requested a review from a team as a code owner September 17, 2025 22:05
@erickzhao erickzhao changed the title refactor: improve internal platform/arch types refactor: improve Options types Sep 17, 2025

@MarshallOfSound MarshallOfSound left a comment

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.

Seems fine at a glance, will be hard to know how it plays until the PR integrating into forge shows up

@felixrieseberg felixrieseberg left a comment

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.

Code looks overall good!

This is some real bike-shedding and feel free to just ignore me, but I don't love Official as a prefix, simply because it doesn't really mean much. We might as well call them Platform and Arch. Just a silly note though.

@erickzhao

Copy link
Copy Markdown
Member Author

I don't love Official as a prefix, simply because it doesn't really mean much. We might as well call them Platform and Arch. Just a silly note though.

Yeah, the naming for the SupportedX vs. OfficialX types is confusing as well! I wonder if we should make the latter more concise and the former more explicit (e.g. Arch vs. ArchWithAll).

Either way, we're not tied to a naming scheme yet as long as we haven't flipped the release job's switch back on, so I'll ask for more Ecosystem WG opinions. :)

@erickzhao erickzhao merged commit df586b9 into main Sep 18, 2025
10 checks passed
@erickzhao erickzhao deleted the better-types branch September 18, 2025 22:01
@continuous-auth

Copy link
Copy Markdown

🎉 This PR is included in version 19.0.0 🎉

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants