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

Throw invoke errors #12

Merged
merged 18 commits into from
Dec 23, 2023
Merged

Throw invoke errors #12

merged 18 commits into from
Dec 23, 2023

Conversation

vixalien
Copy link
Collaborator

@vixalien vixalien commented Dec 11, 2023

By default, only the message Error invoking method ${methodName} is logged when invoking an error. This PR logs throws the GLib.Error correctly if an error happened while executing a function.

I'm creating this as a draft PR because I think the better approach is to throw the GLib.Error (hence interrupting control flow), but I'm still thinking about creating a GLib.Error done

src/types/struct.js Outdated Show resolved Hide resolved
@vixalien vixalien changed the title log invoke errors correctly Throw invoke errors Dec 16, 2023
@vixalien vixalien marked this pull request as ready for review December 16, 2023 16:43
}

export function getGLibError() {
return require("GLib", "2.0").Error;
Copy link
Owner

Choose a reason for hiding this comment

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

I think it's not a good idea to use introspection inside deno_gi as it can lead to recursive dependency on it's self.
is it possible to do this with only using static bindings?

Copy link
Collaborator Author

@vixalien vixalien Dec 17, 2023

Choose a reason for hiding this comment

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

I don't think it's the best idea ever, but it's the best approach.

This is because the GLib.Error must be consistent with other instances of GLib.Error so that we can do instance checks like so:

try {
  const file = Glib.file_get_contents("/path/to");
} catch (err) {
  if (err instanceof GLib.Error && error.matches(GLib.FileError.NOENT)) {
	  console.error("The file was not found");
  } else {
     //... handle error differently
  }
}

The above code would not work otherwise because GLib.Error would be a completely different instance from error.

Copy link
Owner

Choose a reason for hiding this comment

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

okay that makes sense

@vixalien
Copy link
Collaborator Author

That said, do you think it's a good idea to cache repos, so that require(lib, version) always returns the same exact Object instead of creating new ones each time?

This would allow for overriding methods across all imported repos, for example: GLib.Error.toString would be overridden (maybe in #10) and all errors would be overridden at once.

@@ -26,8 +26,7 @@ export function createFunction(info) {
);

if (!success) {
console.error(`Error invoking function ${getName(info)}`);
return;
throw createGError(error);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

createGError should also handle when there is no error, and log that an error happened while trying to invoke the function, as before.

@ahgilak
Copy link
Owner

ahgilak commented Dec 18, 2023

That said, do you think it's a good idea to cache repos, so that require(lib, version) always returns the same exact Object instead of creating new ones each time?

Yeah it's good to cache repos.

src/gi.js Outdated
/**
* @param {string} namespace
* @param {string?} version
* @returns
*/
export function require(namespace, version) {
const key = `${namespace}-${version ?? ""}`;
Copy link
Owner

Choose a reason for hiding this comment

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

creating key this way is not suitable.
if version is not passed, latest version of library available on the system will be loaded, so for example

require("GLib");
require("GLib", "2.0");

should be the same.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if version is not passed, latest version of library available on the system will be loaded, so for example

If the version is not passed, the latest version will always be loaded, which makes the cache return the latest version and the behaviour is correct. What we can do is like GJS, and warn if there are multiple library versions.

Maybe to make the code correct, we can list all the versions and automatically load the latest one.

gjs> imports.gi.Gtk
typein:1:1 warning: Requiring Gtk but it has 2 versions available; use imports.gi.versions to pick one
[object GIRepositoryNamespace]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've updated the code to handle the above scenario, please take a look if you can.

Copy link
Owner

Choose a reason for hiding this comment

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

LGTM. Thanks.

@ahgilak
Copy link
Owner

ahgilak commented Dec 19, 2023

I rebased it on main. is this ready to merge now?

@vixalien
Copy link
Collaborator Author

Thanks, I just added the code to throw a generic error like Error invoking constructor {name} when a function invocation does not succeed but without an error.

Ready for re-review

@ahgilak ahgilak merged commit d4876cd into ahgilak:main Dec 23, 2023
@vixalien vixalien deleted the log-invoke-error branch December 23, 2023 18:18
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