Skip to content
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

[Bug]: Documentation & TypeScript Types inaccuracte for set systemPreferences.appLevelAppearance #30413

Closed
3 tasks done
nathanlesage opened this issue Aug 5, 2021 · 3 comments
Labels
bug 🪲 component/typescript documentation 📓 stale status/confirmed A maintainer reproduced the bug or agreed with the feature

Comments

@nathanlesage
Copy link

Preflight Checklist

Electron Version

13.1.8

What operating system are you using?

macOS

Operating System Version

macOS Big Sur 11.4

What arch are you using?

arm64 (including Apple Silicon)

Last Known Working Electron version

No response

Expected Behavior

After attempting to reset the appLevelAppearance from either light or dark to using the operating system appearance does not work by setting it to unknown. That gave me a "conversion error".

Taking a look into the source code for the systemPreferences API, one can see that there are in fact three possible ways of setting the appLevelAppearance – light, dark and one being undocumented – namely the null value:

if (val->IsNull()) {
*out = nil;
return true;
}

In fact, the appLevelAppearance can successfully be reset by using the following line of code:

systemPreferences.appLevelAppearance = null

I tested this and it worked out of the box.

Actual Behavior

However, upon trying this and set the appLevelAppearance to null, it turned out the Electron types (used for TypeScript) give me an error. But they do not give me an error using the wrong value of 'unknown' which throws the above-mentioned "conversion error".

I recommend the following updates:

  1. Update the Electron typings so that the appLevelAppearance setter accepts null as a valid property to set the app-level appearance to nil
  2. Update the documentation to reflect that you can reset the appLevelAppearance at runtime by setting it to null

Further, I think it might be more appropriate to consolidate getter and setter and, instead of returning "unknown" return the JavaScript representation of nil, namely: null (or even undefined). Alternatively, the setter might accept unknown and convert that internally to nil.

Testcase Gist URL

No response

Additional Information

No response

@clavin clavin added component/typescript documentation 📓 status/confirmed A maintainer reproduced the bug or agreed with the feature labels Aug 7, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2022

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. If you have any new additional information—in particular, if this is still reproducible in the latest version of Electron or in the beta—please include it with your comment!

@github-actions github-actions bot added the stale label Oct 5, 2022
@nathanlesage
Copy link
Author

bump

@MarshallOfSound
Copy link
Member

This is a limitation of typescript itself (see microsoft/TypeScript#2521)

It's not possible for a getter and setter of a property to have incompatible types (which our API does). In order to be TS compliant you can use the setAppLevelAppearance() and getAppLevelAppearance() methods which are typed correctly

@MarshallOfSound MarshallOfSound closed this as not planned Won't fix, can't repro, duplicate, stale Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🪲 component/typescript documentation 📓 stale status/confirmed A maintainer reproduced the bug or agreed with the feature
Projects
None yet
Development

No branches or pull requests

3 participants