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

Add global shell types #13

Merged
merged 1 commit into from
Jan 3, 2024
Merged

Add global shell types #13

merged 1 commit into from
Jan 3, 2024

Conversation

swsnr
Copy link
Collaborator

@swsnr swsnr commented Jan 1, 2024

Closes #12

Add a new global.d.ts module with global shell variables and monkey-patches, see #12. Currently, it's a separate module you'd need to import separately to bring in the global declarations (see changes to the example), but since these are global (i.e. exist regardless of any imports) should I add them to index-ambient?

@swsnr swsnr mentioned this pull request Jan 1, 2024
@schnz
Copy link
Member

schnz commented Jan 1, 2024

since these are global (i.e. exist regardless of any imports) should I add them to index-ambient?

👍 - Agreed. I can't think of a use-case where anyone wouldn't want to import these declarations when using the gnome-shell package. Let's see how the conversation in #12 evolves. But I favor your suggestion over mine.

@swsnr
Copy link
Collaborator Author

swsnr commented Jan 1, 2024

On second thought I think we should not import these globals automatically because these globals don't exist in the environment that runs the prefs.js file.

So if these types were automatically added, auto completion might trick people into using them in their preferences code, only to have nasty surprise at runtime.

As such I think people should have to explicitly opt in to have types for these globals.

For the same reason I'll also move these globals to extensions/global to make clear they're only present for extension code inside Gnome shell, but not for prefs.

I changed this PR to a draft until I get to do that.

@swsnr swsnr marked this pull request as draft January 1, 2024 13:32
@swsnr swsnr marked this pull request as ready for review January 1, 2024 13:49
@swsnr
Copy link
Collaborator Author

swsnr commented Jan 1, 2024

Moved to extensions/global and added an explanation in the commit message.

@JumpLink
Copy link
Collaborator

JumpLink commented Jan 1, 2024

@swsnr Looks good to me, can you please complete the REAMDE.md with the information you have mentioned here? So that people know how and why they can include the globals.

Thanks a lot for your contribution 👍

@swsnr
Copy link
Collaborator Author

swsnr commented Jan 1, 2024

@JumpLink Sure, I'll update the readme file.

Add a separate exported ambient module which defines the globals from
GNOME shell, such as the "global" object or the monkey-patched string
class.

Do not automatically import this module in the index modules, because
these globals are not present in the environment that runs prefs.js
code.  As such, automatically importing them might trick users into
using these globals in their prefs.js code, e.g. through auto
completion, with a nasty surprise at runtime.

Hence, users should explicitly opt-in to bring these types into their
global environment, and hopefully be aware that they can't be used in
prefs.js code.

Closes #12
@swsnr
Copy link
Collaborator Author

swsnr commented Jan 2, 2024

Update, and I also fixed the definition of String.format. Should be ready now.

@JumpLink JumpLink merged commit fe9e6ee into gjsify:main Jan 3, 2024
@JumpLink
Copy link
Collaborator

JumpLink commented Jan 3, 2024

@swsnr and @schnz thank you both!

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.

Global types
3 participants