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

Fix warning on GLib >=2.79.2 #981

Merged
merged 1 commit into from
Jul 11, 2024
Merged

Fix warning on GLib >=2.79.2 #981

merged 1 commit into from
Jul 11, 2024

Conversation

C-512L
Copy link
Contributor

@C-512L C-512L commented Jul 3, 2024

No description provided.

@amezin
Copy link
Member

amezin commented Jul 3, 2024

Your change isn't compatible with GNOME versions older than 46. ddterm master branch currently supports GNOME >= 43. Between supporting more GNOME versions, and getting rid of a warning, I will obviously choose compatibility.

But you likely can make it work for all GNOME versions using https://gjs-docs.gnome.org/gjs/overrides.md#gi-require

@C-512L
Copy link
Contributor Author

C-512L commented Jul 3, 2024

I have been able to backport it to all systems using ESM on my own branch, but I might have stumbled on a ESM translator edge case being unable to translate import Gi from 'gi'; correctly. I tried to look at that code of translate-esm.js but unfortunalety I haven't found much documentation about using gi.require() on pre-ESM GJS to make a patch by myself.

@amezin
Copy link
Member

amezin commented Jul 3, 2024

There's also sed preprocessing step to solve issues like that:

// BEGIN ESM
if (this.source.addNotification)
this.source.addNotification(this);
else
this.source.showNotification(this);
// END ESM
// BEGIN !ESM
this.source.showNotification(this);
// END !ESM
(although here it's used just to remove unnecessary if from legacy version)

You should be able to use legacy imports between // BEGIN !ESM and // END !ESM lines

@C-512L
Copy link
Contributor Author

C-512L commented Jul 3, 2024

I didn't feel sure about adding a lint exception, but I guessed it would be a better solution than adding dead code to the non-ESM build or duplicating the preprocessed code. Feedback is welcomed.

ddterm/shell/sd_journal.js Outdated Show resolved Hide resolved
ddterm/shell/sd_journal.js Outdated Show resolved Hide resolved
@C-512L C-512L changed the title Fix GLib warning Fix warning on GLib >=2.79.2 Jul 10, 2024
ddterm/shell/sd_journal.js Outdated Show resolved Hide resolved
ddterm/shell/sd_journal.js Show resolved Hide resolved
@amezin amezin merged commit de78982 into ddterm:master Jul 11, 2024
14 checks passed
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.

None yet

2 participants