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

Debugging when a package service is incorrect #995

Merged
merged 1 commit into from
May 10, 2024
Merged

Conversation

mauricioszabo
Copy link
Contributor

This PR is basically to add some debugging info for providers and consumers. Currently, when a package registers as a service provider or a service consumer, and doesn't actually implements the right interface (a function, basically) the package loader silently ignores it. This commit adds a warning to console so that we know a package did the wrong thing.

This PR is basically to add some debugging info for providers and
consumers. Currently, when a package registers as a service provider or
a service consumer, and doesn't actually implements the right interface
(a function, basically) the package loader silently ignores it. This
commit adds a warning to console so that we know a package did the wrong
thing.
@DeeDeeG
Copy link
Member

DeeDeeG commented May 6, 2024

The CI failure is probably unrelated, see #996 hopefully for the fix.

EDIT: Oof, never mind, #996 still needs some work. But hopefully a fix will be available soon, and yeah looks like this it's not this PR's fault for CI failing. EDIT 2: See #997 instead for a CI fix.

Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

I'm not very familiar with services/providers/consumers, so if you want to wait for another reviewer that would make sense, but this implementation looks extremely simple and low-stakes, just adding two console.warn()s.

I'm not super familiar with the surrounding code to be sure we will have a this.name, name, version and methodName vars defined, but other than that I see no reason why this shouldn't work...

👍 LGTM

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

Low stakes and provides us even more information.
Would love to see if this information could be integrated into deprecation cop somehow to make it more viewable, but otherwise looks good to me

@DeeDeeG
Copy link
Member

DeeDeeG commented May 8, 2024

Should I merge in the macOS CI fix to this branch and re-run CI to see if it passes? Or just go ahead and merge this one as-is? I think it's very nearly impossible for this to cause errors unless it so happens to try and access a var or property that's not defined. Then again, I doubt that this code path is exercised in our existing specs either.

@confused-Techie
Copy link
Member

@DeeDeeG There's no harm in doing so, but I don't think it's needed. With both our approvals I think we are good to merge

@DeeDeeG DeeDeeG merged commit c6f2d8d into master May 10, 2024
101 of 103 checks passed
@mauricioszabo mauricioszabo deleted the debug-for-services branch May 13, 2024 16:10
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