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

Adapt typings for Gnome 47 release #51

Merged
merged 1 commit into from
Sep 22, 2024
Merged

Adapt typings for Gnome 47 release #51

merged 1 commit into from
Sep 22, 2024

Conversation

schnz
Copy link
Member

@schnz schnz commented Sep 20, 2024

Based on the changes listed here: https://gjs.guide/extensions/upgrading/gnome-shell-47.html


I am going to test integration with gTile now and will report back whether I stumbled upon any errors.

@schnz
Copy link
Member Author

schnz commented Sep 20, 2024

At first, I struggled to solve errors due to double-exports. They were a direct result of dependency conflicts (i.e. some dependencies used beta.15 packages, while others used beta.16 packages and those imports were then intermixed).

After the the latest commit 51eb4a5, everything works :).

The only error that popped-up when upgrading the types was - as expected - the changed return type of fillPreferencesWindow (which is a breaking change, although we could continue to allow void if we care for backward compatibility)


Bottom line: This PR is good to be merged IMO. All typings work flawlessly with the gTile extension.

@swsnr swsnr self-assigned this Sep 20, 2024
packages/gnome-shell/src/ui/messageList.d.ts Outdated Show resolved Hide resolved
packages/gnome-shell/src/misc/util.d.ts Outdated Show resolved Hide resolved
@Totto16 Totto16 added this to the Gnome 47 milestone Sep 20, 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 believe the new types for getPreferencesWidget and fillPreferencesWindow are overly strict.

I've checked the GNOME Shell sources, and all GNOME Shell does with the return value of these functions, is awaiting them.

So they can actually return not just a Promise, but also any then-able object, or the corresponding plain type. Off the top of my head I don't know whether Typescript has a generic type to represent the permitted type for an await'ed expression, tho.

That said, then-able objects are probably not an issue, because we don't have a library ecosystem and don't need to handle non-native or foreign promises.

But fillPreferencesWindow should return Promise<void> | void nonetheless. Otherwise typescript would require a Promise being returned, but I believe the vast majority of fillPreferencesWindow implementations aren't async and do not actually need to return a Promise. Being forced to return Promise<void> would be rather inconvenient for many extensions I presume.

@Totto16
Copy link
Collaborator

Totto16 commented Sep 20, 2024

I believe the new types for getPreferencesWidget and fillPreferencesWindow are overly strict.

I've checked the GNOME Shell sources, and all GNOME Shell does with the return value of these functions, is awaiting them.

So they can actually return not just a Promise, but also any then-able object, or the corresponding plain type. Off the top of my head I don't know whether Typescript has a generic type to represent the permitted type for an await'ed expression, tho.

That said, then-able objects are probably not an issue, because we don't have a library ecosystem and don't need to handle non-native or foreign promises.

But fillPreferencesWindow should return Promise<void> | void nonetheless. Otherwise typescript would require a Promise being returned, but I believe the vast majority of fillPreferencesWindow implementations aren't async and do not actually need to return a Promise. Being forced to return Promise<void> would be rather inconvenient for many extensions I presume.

I agree with that, and you can await x() if x : Promise<void> | void and if you have eslint it warns you, when you do x() So it should be doable for extensions. These types are not meant to be super strict, many write extensions in js, since they have to many type errors, but the extension has no errors at runtime. It got better the last few releases, but we should mostly use more loose types, so that the types are easier to use.

@schnz
Copy link
Member Author

schnz commented Sep 20, 2024

I believe the new types for getPreferencesWidget and fillPreferencesWindow are overly strict.

I agree on the fillPreferencesWindow part. As for getPreferencesWidget: The type widened. No regression here.


Being forced to return Promise would be rather inconvenient for many extensions I presume.

Yeah - although its sufficient to add an async before the method name.

I don't know our policy on breaking changes to be honest. I tried to stay as close to the upstream API as possible.

At the end of the day its a trade-off between (i) staying compatible with the upstream API and (ii) deviate deliberately from time to time in favor of ease-of-use/convenience. I have no strong opinion on that, other than option (ii) is hard(er) to maintain because we have to know whether the deviation breaks anything in the upstream implementation (e.g. when it's using .then() on the returned value).

Since both of you @swsnr and @Totto16 agree on the matter, I've added back the void return type 👍

@swsnr
Copy link
Collaborator

swsnr commented Sep 20, 2024

I agree on the fillPreferencesWindow part. As for getPreferencesWidget: The type widened. No regression here.

I was referring to the GNOME Shell source code, not to the previous major version of these typings.

I don't know our policy on breaking changes to be honest. I tried to stay as close to the upstream API as possible.

I'm not sure what you mean with this sentence. It's not as if we have a choice 😅 We must follow what GNOME Shell does.

And all I was trying to say is that GNOME Shell just awaits the return values of these two functions, and since await has pretty generous type restrictions (see MDN), the accurate return type for either of these methods would probably be Promise<T> | PromiseLike<T> | T, since that's about what can be used as an expression for await (roughly, we still ignore nested promises here, which the type doesn't allow, but which await would happily unfold at runtime).

IOW, I don't care about breaking changes wrt to the previous major version of these typings, I care about modelling the types of the current major release accurately.

@swsnr swsnr removed their assignment Sep 20, 2024
@schnz
Copy link
Member Author

schnz commented Sep 21, 2024

I was referring to the GNOME Shell source code, not to the previous major version of these typings.

Me too

I'm not sure what you mean with this sentence. It's not as if we have a choice 😅 We must follow what GNOME Shell does.

I was referring to the proposed change of the method signature from Promise<void> (as it is in upstream) to Promise<void> | void - which is a deviation from upstream. Adhering to upstream implies a breaking change because the previous return type was simply void.


@Totto16 - Feel free to resolve the discussions above at your discretion. I've added the changes.

@swsnr
Copy link
Collaborator

swsnr commented Sep 21, 2024

@schnz Oh, I didn't notice the jsdoc comments. Most parts of Gnome shell don't have them, so I didn't bother to check the signature but instead went straight to the call site to see how it's actually used 😅

So disregard my comments. If we have type info we should probably follow it, shouldn't we? And just use Promise<void>?

@swsnr
Copy link
Collaborator

swsnr commented Sep 22, 2024

@Totto16 I've taken the liberty to merge this already 🙂

@swsnr swsnr merged commit f79c918 into v47.0.0 Sep 22, 2024
2 checks passed
@swsnr swsnr deleted the update-types-v47 branch September 22, 2024 10:40
@Totto16
Copy link
Collaborator

Totto16 commented Sep 25, 2024

@Totto16 I've taken the liberty to merge this already 🙂

No worries, my change requests were non blocking 👍🏼

This was referenced Sep 25, 2024
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.

3 participants