-
Notifications
You must be signed in to change notification settings - Fork 824
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(scope-zone): new scope manager to support async operations in web #461
feat(scope-zone): new scope manager to support async operations in web #461
Conversation
Codecov Report
@@ Coverage Diff @@
## master #461 +/- ##
==========================================
- Coverage 94.39% 92.43% -1.97%
==========================================
Files 120 143 +23
Lines 5749 6863 +1114
Branches 533 597 +64
==========================================
+ Hits 5427 6344 +917
- Misses 322 519 +197
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really cool! I've given a first pass that caught mostly extra log entries, style changes, etc.
I'd like to understand and review the design and core code a bit more carefully though.
Could you provide a longer-form JSDoc comment at the start of the zone scope manager that explains its design at a high level, including what data it stores, how it attaches / unattaches to listeners and timeouts, etc.?
I'm also curious as to why the extra complexity with binding is needed vs. relying more heavily on Zones themselves (i.e. delegating the binding to the Zone binding functions)
packages/opentelemetry-scope-zone-peer-dep/src/ZoneScopeManager.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-scope-zone-peer-dep/src/ZoneScopeManager.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-scope-zone-peer-dep/src/ZoneScopeManager.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-scope-zone-peer-dep/src/ZoneScopeManager.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-scope-zone-peer-dep/src/ZoneScopeManager.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-scope-zone-peer-dep/src/ZoneScopeManager.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-scope-zone-peer-dep/src/ZoneScopeManager.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-scope-zone-peer-dep/src/ZoneScopeManager.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-scope-zone-peer-dep/src/ZoneScopeManager.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-scope-zone-peer-dep/src/ZoneScopeManager.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome that this now supports parallel async operations. I think this is really coming together.
I made some nit-picky type comments, but also asked a question about the event listeners - I'd like to understand that part better and help us think through whether it's needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on this @obecny !! I think this is in a good place to merge in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Which problem is this PR solving?
Short description of the changes
This PR adds new ZoneScopeManager for web that will support async operation - will keep the reference to scope (span) between asynchronous operations. This will allow to build more plugins for web. It is created into 2 packages. One is bundled together with
zone.js
and second allows to use your ownzone.js
if user already has it (for example when building angular apps)Updated the corresponding readme too.