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

Mark gnome types for 47 #53

Merged
merged 9 commits into from
Sep 26, 2024
Merged

Mark gnome types for 47 #53

merged 9 commits into from
Sep 26, 2024

Conversation

Totto16
Copy link
Collaborator

@Totto16 Totto16 commented Sep 25, 2024

I checked all modules I use in my extension, and as expected I didn't find many changes.

The last commit may be controversial, as discussed in #51 in several comments.

The method fillPreferencesWindow is now async, see https://gitlab.gnome.org/GNOME/gnome-shell/-/commit/0f30bfdd531ed0e8ed78bda74d51b7f22872a31b#

But I believe it's better to have a Promise<void> | void type, even if the doctoring says, it has to be a Promise, it can also be no promise as the JavaScript usage is only with await fillPreferencesWindow() which also works with a non async version of the function. If it would use .then() or similar this wouldn't work, but I believe it's better to also allow void

See also #49 (comment)

@Totto16 Totto16 changed the base branch from main to v47.0.0 September 25, 2024 22:14
@Totto16 Totto16 added this to the Gnome 47 milestone Sep 25, 2024
Copy link
Collaborator

@swsnr swsnr left a comment

Choose a reason for hiding this comment

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

I think I'd prefer if we strictly adhered to the types GNOME Shell defines, meaning, fillPreferencesWindow should be Promise<void> only, for two reasons, one more on a conceptual level, and another quite specific one.

First, I think it's great that GNOME Shell adds these annotations, and I believe on the long run we should investigate into using these to auto-generate these types, at least partially (Typescript can derive types from jsdoc comments). If we go down this route we'd have no flexibility to adapt declarations, so I believe it's conceptually not a good idea to deviate from upstream in cases where upstream does provide explicit type information.

And then, in this particular case, I do not understand the implications for backwards compatibility. Let's assume we stick to GNOME Shell and declare a return type of Promise<void>. Then I see three scenarios in extensions:

  1. You're branching off for every new GNOME major version, and then use the corresponding types in each respective branch (that's what I tend to do in so far I bother with older GNOME versions): In this case backwards compatibility of the GNOME 47 types won't matter, since you'd be using the GNOME 46 types for your 46 code anyway. Things just work.

  2. You're supporting GNOME 46 and GNOME 47 from one branch, and

    a. you're using the GNOME 46 types. In this case the return type is void and you'll keep returning nothing, which accidentally also works under GNOME 47 since it just awaits the returned promise. Things just work, albeit arguably with some level of unsafety.

    b. Or you're using the GNOME 47 types. In this case you change the return type to Promise.resolve() to return Promise<void>, to comply with the Promise<void> return type, as expected by GNOME 47. Since GNOME 46 expects no return value from fillPreferencesWindow it'll just ignore the empty promise returned from the method. Things will just work, with perhaps a bit less unsafety.

But in all these cases I don't see how a Promise<void> | void return type would help with backwards compatibility 🤔

It's just convenience so that you don't have to add return Promise.resolve() to fillPreferencesWindow in all your extensions, but I'd argue that omitting this isn't explicitly supported by upstream on GNOME 47, so I tend to believe we shouldn't support it either 🙂

@Totto16 Totto16 force-pushed the mark-gnome-types-for-47 branch from e5bc692 to a1a5431 Compare September 26, 2024 15:04
@Totto16 Totto16 requested a review from swsnr September 26, 2024 15:04
@Totto16
Copy link
Collaborator Author

Totto16 commented Sep 26, 2024

I think I'd prefer if we strictly adhered to the types GNOME Shell defines, meaning, fillPreferencesWindow should be Promise<void> only, for two reasons, one more on a conceptual level, and another quite specific one.

First, I think it's great that GNOME Shell adds these annotations, and I believe on the long run we should investigate into using these to auto-generate these types, at least partially (Typescript can derive types from jsdoc comments). If we go down this route we'd have no flexibility to adapt declarations, so I believe it's conceptually not a good idea to deviate from upstream in cases where upstream does provide explicit type information.

And then, in this particular case, I do not understand the implications for backwards compatibility. Let's assume we stick to GNOME Shell and declare a return type of Promise<void>. Then I see three scenarios in extensions:

1. You're branching off for every new GNOME major version, and then use the corresponding types in each respective branch (that's what I tend to do in so far I bother with older GNOME versions): In this case backwards compatibility of the GNOME 47 types won't matter, since you'd be using the GNOME 46 types for your 46 code anyway.  Things just work.

2. You're supporting GNOME 46 and GNOME 47 from one branch, and
   a. you're using the GNOME 46 types.  In this case the return type is `void` and you'll keep returning nothing, which accidentally also works under GNOME 47 since it just awaits the returned promise.  Things just work, albeit arguably with some level of unsafety.
   b. Or you're using the GNOME 47 types.  In this case you change the return type to `Promise.resolve()` to return `Promise<void>`, to comply with the `Promise<void>` return type, as expected by GNOME 47.  Since GNOME 46 expects no return value from `fillPreferencesWindow` it'll just ignore the empty promise returned from the method.  Things will just work, with perhaps a bit less unsafety.

But in all these cases I don't see how a Promise<void> | void return type would help with backwards compatibility 🤔

It's just convenience so that you don't have to add return Promise.resolve() to fillPreferencesWindow in all your extensions, but I'd argue that omitting this isn't explicitly supported by upstream on GNOME 47, so I tend to believe we shouldn't support it either 🙂

I agree with you in #49 and here too, so I removed that commit 👍🏼

@swsnr swsnr merged commit 381db6a into v47.0.0 Sep 26, 2024
3 checks passed
@swsnr swsnr deleted the mark-gnome-types-for-47 branch September 26, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants