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

feat: Add debugName parameter to YAJsonIsolate. #1118

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

misha
Copy link

@misha misha commented Feb 14, 2025

What kind of change does this PR introduce?

This is a new feature for YAJsonIsolate.

In testing environments, it's easy to have multiple clients open. Specifically, I usually have an admin client and a client for the current user, but I can't tell which is which because all isolates have the same name. When an error occurs in the isolate, it takes a lot of effort to figure out which client produced it.

I would like to be able to rename isolates, specifically for use in debugging.

What is the current behavior?

Currently, all isolates spawned have the name _compute.

Here's a screenshot of what it looks like in VSCode.

before_change

What is the new behavior?

There's now a debugName constructor parameter on YAJsonIsolate. The user can add the library to their package and instantiate an instance with their desired debugName if they want to access it.

Here's a screenshot of what it looks like in VSCode, after setting the name to my_isolate.

after_change

Additional context

Two questions:

  1. There's also a web implementation in _isolates_web.dart. Does this now need the same (albeit unused) constructor parameter?
  2. I didn't write a test. I could store and expose the underlying Isolate instance to verify it gets created with the specified debugName, but then I have to remember to unassign it so I'd rather just not store it. If a test is required, let me know what it should do.

@coveralls
Copy link

coveralls commented Feb 14, 2025

Pull Request Test Coverage Report for Build 13339988921

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 73.739%

Totals Coverage Status
Change from base Build 13051463855: 0.01%
Covered Lines: 2777
Relevant Lines: 3766

💛 - Coveralls

@dshukertjr
Copy link
Member

Thanks for the PR! A test would be great! I don't think we need to worry about the web implementation!

@Vinzent03
Copy link
Collaborator

I'm pretty sure we have to change the web part as well. Otherwise the signature is different and the build should fail for web.

@misha
Copy link
Author

misha commented Feb 15, 2025

Thanks for the PR! A test would be great! I don't think we need to worry about the web implementation!

Can you mention how the test should be implemented?

The only way I can think to test this is to store the Isolate instance, expose it for testing, and then validate the name on the isolate itself. This would require test-visible access to the isolate, ie. via @visibleForTesting. Presently, the implementation doesn't even store the spawned isolate, much less expose it in any way. I would say it's not worth the risk to test a non-critical, debugging feature.

I'm pretty sure we have to change the web part as well. Otherwise the signature is different and the build should fail for web.

Yep, that sounds right to me. I added a commit to make the constructor signatures identical.

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