-
Notifications
You must be signed in to change notification settings - Fork 190
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
[REFACTOR] Integrate DebugRenderTree #1192
Conversation
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.
Overall seems good, a few high level questions though:
- Can you add tests for this using the highest level APIs possible? (rendering a tree of things, asserting on the deep state of the render tree)
- Can you make a draft PR to Ember showing the integration side (so we can get a feel for how it looks over there)?
- Can you update the PR description to be clearer about what specifically is breaking (e.g. these props / classes were removed/renamed/etc)?
packages/@glimmer/interfaces/lib/components/component-manager.d.ts
Outdated
Show resolved
Hide resolved
packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/components.ts
Outdated
Show resolved
Hide resolved
93ab860
to
f434e15
Compare
Here is the integration PR in Ember: emberjs/ember.js#19261 |
Integrates the DebugRenderTree into Glimmer VM directly, so it doesn't have to be managed by individual component managers. Major changes: 1. `getDebugRenderTreeName` API added if managers need to override the name provided by `getDebugName` 2. `getCustomDebugRenderTree` API added, allows managers like `outlet` and `mount` to specify custom render trees via a list of render nodes. 3. TemplateOnly components can now integrate in the render tree without having to optionally have `createArgs` or `createInstance` capabilities.
f434e15
to
d3d097d
Compare
if (didError !== true) { | ||
endTrackingTransaction!(); | ||
} |
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 seems a tad surprising, since runInTrackingTransaction
always calls beginTrackingTransaction
why wouldn't it always call endTrackingTransaction
?
Is it because we already call resetTracking
(basically in our callsite)?
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.
Exactly that. With the current structure of things, there is a case where this can happen unfortunately. I think we can work on restructuring it in the future to make it a bit more bulletproof.
Integrates the DebugRenderTree into Glimmer VM directly, so it doesn't
have to be managed by individual component managers.
Major changes:
getDebugRenderTreeName
API added if managers need to override thename provided by
getDebugName
getCustomDebugRenderTree
API added, allows managers likeoutlet
and
mount
to specify custom render trees via a list of render nodes.having to optionally have
createArgs
orcreateInstance
capabilities.
Breaking changes:
env.debugRenderTree
has been made optional, it could potentially be undefined and will not assert if it is notDebugRenderTree#setTemplate
has been removed