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

Next version: GNOME Shell 47 #49

Merged
merged 29 commits into from
Nov 3, 2024
Merged

Next version: GNOME Shell 47 #49

merged 29 commits into from
Nov 3, 2024

Conversation

JumpLink
Copy link
Collaborator

@JumpLink JumpLink commented Jul 19, 2024

I suggest that we leave this PR open until GNOME Shell 47 is released. Until then, we can publish releases with the next tag, I have already released the current state of this PR this way.

I hope this helps to prepare your extensions for the next release and fix the types from here along the way ;)

Changes

  • General preparations for the next GNOME release by updating the dependent types for it.
  • Upgrade yarn
  • Upgrade other dependencies

@swsnr
Copy link
Collaborator

swsnr commented Aug 30, 2024

@JumpLink Can we go back to 3.x typings? These 4.0.0 beta types (presumably generated after the merge with gi.ts?) are still lacking a bunch of promisified overloads, and it just doesn't seem right for a stable typing to depend on beta types which still regress over the previous stable types.

See also #39 (comment)

@Totto16
Copy link
Collaborator

Totto16 commented Aug 31, 2024

@swsnr I think it is better, to fix this issue (gjsify/ts-for-gir#171) and don't use older types, since they lack some other improvements. 🤷🏼‍♂️

@swsnr
Copy link
Collaborator

swsnr commented Sep 19, 2024

@JumpLink Now that Gnome 47 is out, how do we proceed here?

@schnz
Copy link
Member

schnz commented Sep 19, 2024

I just checked out https://gjs.guide/extensions/upgrading/gnome-shell-47.html

Gnome changed the method signature of getPreferencesWidget and fillPreferencesWindow in prefs.js to return a Promise. Would be great to have those changes as well before merging:

--- a/prefs.d.ts
+++ b/prefs.d.ts
@@ -11,10 +11,10 @@ export class ExtensionPreferences extends ExtensionBase {
      * Get the single widget that implements
      * the extension's preferences.
      *
-     * @returns {Gtk.Widget}
+     * @returns {Gtk.Widget|Promise<Gtk.Widget>}
      * @throws {GObject.NotImplementedError}
      */
-    getPreferencesWidget(): Gtk.Widget;
+    getPreferencesWidget(): Gtk.Widget | Promise<Gtk.Widget>;

     /**
      * Fill the preferences window with preferences.
@@ -24,7 +24,7 @@ export class ExtensionPreferences extends ExtensionBase {
      *
      * @param {Adw.PreferencesWindow} window - the preferences window
      */
-    fillPreferencesWindow(window: Adw.PreferencesWindow): void;
+    fillPreferencesWindow(window: Adw.PreferencesWindow): Promise<void>;
 }

@swsnr
Copy link
Collaborator

swsnr commented Sep 20, 2024

@schnz Be the change you want to see 😉

We'd appreciate if you could take this branch and finish it (mostly fixing the merge conflicts), and add the above change.

I'll review it, and I think I'd also be able to publish a preliminary release for 47 to Npm. 🙂

@schnz
Copy link
Member

schnz commented Sep 20, 2024

Sure - I can add the changes (in addition to everything else that's mentioned in the referenced CHANGELOG)

But I am at work - so I won't be able to do that until tonight or Saturday morning.

@swsnr
Copy link
Collaborator

swsnr commented Sep 20, 2024

@schnz That'd be very much appreciated 🙏 Take all the time you need, that's still likely a lot faster than any of us can do this 😅

@JumpLink
Copy link
Collaborator Author

I am now updating all generated gir types in ts-for-gir to their latest version so that we can update them here once again

@swsnr
Copy link
Collaborator

swsnr commented Sep 20, 2024

@JumpLink 🎉 🙏

@Totto16
Copy link
Collaborator

Totto16 commented Sep 20, 2024

We need to manually update all / or at least those who changed @version 46 types, I'll be happy to help there, I'll read through the changes and test my extension in gnome 47 and then I can help with a few things too 👍🏼

@swsnr
Copy link
Collaborator

swsnr commented Sep 20, 2024

I think the aforementioned change to ExtensionPrefs is the only significant change. So let's perhaps get a version with updated dependencies out first?

@JumpLink
Copy link
Collaborator Author

@swsnr @Totto16 If you think that you won't be able to implement it so quickly, we can do it that way, otherwise you are welcome to create a PR in this branch / PR. I am currently publishing the updated types on NPM and then updating them here again, I would then be done for now and would hand over the rest to you both?

@Totto16
Copy link
Collaborator

Totto16 commented Sep 20, 2024

@swsnr @Totto16 If you think that you won't be able to implement it so quickly, we can do it that way, otherwise you are welcome to create a PR in this branch / PR. I am currently publishing the updated types on NPM and then updating them here again, I would then be done for now and would hand over the rest to you both?

I'll do it tomorrow, so IMO we should wait for some testing until merging into main, even if we did merge everything into main with gnome 46, but having a feature branch v47 is a good idea. If we publish types with the progress, it's also easy to test things and create PR's that are directed to this branch and no to main. But in general I am open to do it how the majority likes it better.

@swsnr
Copy link
Collaborator

swsnr commented Sep 20, 2024

@Totto16 I don't have much time this weekend, so if you can work on these types this weekend, then do it the way that's best for you 👍 We can still discuss this later on.

I've released my extensions for 47 already, based on the 46 types here, but I haven't seen any runtime issues so far, so I believe with this release the impact on the typings is really small (unlike, say 45 to 46).

@JumpLink
Copy link
Collaborator Author

I have published the current status of this PR on NPM for testing

@JumpLink
Copy link
Collaborator Author

@swsnr Oh sorry my mistake, you are right

@Totto16
Copy link
Collaborator

Totto16 commented Sep 25, 2024

I now checked the types extensively with my extension, and they seem to work correctly in all cases.

There is only one caveat: I try to support more versions of the extension in one branch, so I use a non async version of fillPreferencesWindow in the code, it works in the runtime but the types say it has to be a promise, we already discussed this in #51 and this also belongs to #53.

For more reference on this see both PRs from above, 😉

We should further discuss this, as I find it good to respect doc strings of the actual implementation, but also be reasonable about it.

It isn't required to change, but I'd like to be backwards compatible in at least some way 😓

@schnz and @swsnr what do you think of this, a we / you already discussed this in #51 ?

@Totto16 Totto16 mentioned this pull request Sep 25, 2024
@schnz
Copy link
Member

schnz commented Sep 26, 2024

Hmm, like I said in #51

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

The way I see it:

  • Technically, it's a breaking change by Gnome.
  • Practically, I see the point that it doesn't really impact any existing extensions (due to what @swsnr metionend, that the upstream implementation only awaits the result and does not use the native Promise API)

However, allowing void is, again, technically wrong and could be incompatible with upstream if, e.g., they decide to use .then() or similar stuff.

I don't have a good solution here to be honest. I could argue for and against both options in one way or another

  • (i) deviating from upstream API and widen our typings as long as it doesn't break the upstream implementation (hard to keep track of over time)
  • (ii) stick strictly to upstream at the expense of forcing extensions to transitively inherit breaking changes (the only question is: how strict are we in declaring such minor things as breaking change)

I, for myself, have choosen option (ii) and the new release of gTile is no longer compatible with Gnome versions below 47, although it would work flawlessly on them.

@swsnr
Copy link
Collaborator

swsnr commented Sep 26, 2024

As said above, I tend for option ii as well, more so, because I'd really like to look into some kind of automation based on the jsdoc type annotations from upstream (in my dream world GNOME Shell itself would use typescript and we'd get types for free ☁️ 🫠 )

@JumpLink
Copy link
Collaborator Author

I also think it makes sense if we start to port the missing types in GNOME Shell as JSDoc types and build something to read them out. It's best to build something to generate the types first, then see what's not working well and then start improving that in GNOME Shell itself.

But then a separate issue / PR / branch makes sense. For now we can publish the types for version 47 as before.

What do you think @swsnr @Totto16 @schnz ?

@swsnr
Copy link
Collaborator

swsnr commented Sep 26, 2024

Automation would be very long term goal. Annotations are still scarce in upstream, and I don't really have much time to work on this any time soon anyway 😅

So let's just continue with our current process.

@JumpLink
Copy link
Collaborator Author

Yes I agree, maybe we can also have something like an allow list for files with JSDoc types that are tested and working, so that this runs as a by-product. Also the GNOME Shell developers themselves have often mentioned that they would prefer to maintain the types directly in GNOME Shell, so they would be co-operative. In the best case scenario, this would mean less work for everyone involved in the long term

@schnz
Copy link
Member

schnz commented Sep 26, 2024

I like the idea of automation. I could see myself implementing a prototype in a time-boxed manner (in 1 or 2 months from now) to see if type extraction in a semi-automated way is possible. But this would be no more than a little digging into the topic and a breakdown of steps that would have to be done in order to integrate this to gisify/gnome-shell.

This would imply that we strictly follow JSDoc types from upstream, where available. This would also have the effect that we would have to discuss type widening changes, like the one proposed by @Totto16 here, directly in the gnome-shell repository with the upstream developers. In this instance, I would assume that they are open to widening the return type to avoid an otherwise totally unnecessary breaking change.

@swsnr
Copy link
Collaborator

swsnr commented Sep 26, 2024

@schnz @JumpLink So the main point for now would be that we follow upstream's annotations strictly, and rather work with upstream in cases where we'd like to have a different (e.g. wider) annotation?

@JumpLink
Copy link
Collaborator Author

@swsnr Good question! I think we should create an issue on the GNOME Shell GitLab instance and discussing these questions there

@JumpLink
Copy link
Collaborator Author

I think we can merge this now?

@swsnr
Copy link
Collaborator

swsnr commented Oct 30, 2024

I think so, yes 👍

@Totto16
Copy link
Collaborator

Totto16 commented Oct 30, 2024

I would agree too 👍🏼

@JumpLink JumpLink merged commit 774a631 into main Nov 3, 2024
2 checks passed
@JumpLink JumpLink deleted the v47.0.0 branch November 3, 2024 15:41
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.

4 participants