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

Propagate context state in Glimmer VM #2

Merged
merged 4 commits into from
Sep 28, 2023

Conversation

kevinkucharczyk
Copy link
Collaborator

@kevinkucharczyk kevinkucharczyk commented Sep 27, 2023

https://customerio.slack.com/archives/C3EPR6SGH/p1695703353394239

This moves away from the DebugRenderTree for our context implementation, to a custom implementation based on Glimmer VM operations.

The DebugRenderTree that I initially used is really only meant to be used by Ember Inspector. Because of all the extra tracking that it does, there may be performance implications from running it in production, so it's best for us to avoid using it in real apps.

To find an alternative solution, I started digging into the Glimmer VM and ended up with a fork of it here: https://github.com/customerio/glimmer-vm/tree/provide-consume-context

Essentially, I added a very simplified version of the DebugRenderTree just for tracking our context provider parent/child relationships as components render. I don't do any extra bounds/nodes/arguments tracking that DebugRenderTree does, which makes this implementation lighter in terms of performance load.

However, we can't easily consume the Glimmer VM fork from above. Ember.js actually bundles the Glimmer VM code in its own output when it gets built. Meaning that in the output it's not a regular npm dependency, it's part of the Ember source - so we can't just point our package.json to a different Glimmer VM.

In order to use it, we'd also need to fork Ember.js, built it, and publish the build in our own registry. We'd then need to make sure all apps that consume our ember-context addon are on that Ember fork.

Inspired by some Ember polyfill addons, I was able to arrive at the PR you see here. In this PR I hook into the classes that are exposed in Ember, and extend them to include our custom context tracker.

@kevinkucharczyk kevinkucharczyk marked this pull request as ready for review September 27, 2023 06:22

// GetComponentSelf opcode
// https://github.com/glimmerjs/glimmer-vm/blob/68d371bdccb41bc239b8f70d832e956ce6c349d8/packages/%40glimmer/vm/lib/opcodes.ts#L196
if (opcode.type === 90) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be worth creating an enum to have these organized together and to have a single place to update them everywhere

Comment on lines +37 to +38
// TODO: Can the Provider component be built into Glimmer? Should we do this
// with some sort of manager?
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind expanding on this?

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 the ProvideConsumeContextContainer ever makes it into glimmer vm, we probably can't just throw in a ContextProvider component in there. Glimmer VM doesn't hold any such components, any actual components are defined on top of it in other packages.
Instead, we could maybe introduce a special "manager", like component managers or helper managers. That might allow us to ask component instances whether they're provider-like without having to write concrete implementations of such providers.

Comment on lines +27 to +29
while (!this.stack.isEmpty()) {
this.stack.pop();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a benefit/requirement to unwind the stack rather than clearing it? I'm guessing there are some sort of side-effects which require us to unwind it, but it's non-obvious

Copy link
Collaborator Author

@kevinkucharczyk kevinkucharczyk Sep 28, 2023

Choose a reason for hiding this comment

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

Good question!
To be honest, I copied a lot from DebugRenderTree, and it's not obvious there either why it's done like that. From what I understand, the Stack implementation (https://github.com/glimmerjs/glimmer-vm/blob/68d371bdccb41bc239b8f70d832e956ce6c349d8/packages/%40glimmer/util/lib/collections.ts#L37) also updates current when you pop. I suppose we could manually reset current ourselves here, but I wonder if there are some side effects that access the current stack item as it's being cleared.

Another interesting case is: there are other implementations of stacks in glimmer, like https://github.com/glimmerjs/glimmer-vm/blob/68d371bdccb41bc239b8f70d832e956ce6c349d8/packages/%40glimmer/runtime/lib/vm/stack.ts#L53. This one also reaches elsewhere to update the current state when items are popped.

The Stack implementations also don't actually expose the internal array at all, and don't provide a "reset", so - using those classes - it's not possible to clear the stack any other way.

As I'd like to stay as close to Glimmer implementations as possible, I'll leave this as is.

// https://github.com/glimmerjs/glimmer-vm/blob/68d371bdccb41bc239b8f70d832e956ce6c349d8/packages/%40glimmer/util/lib/collections.ts#L18
export class Stack<T> {
private stack: T[];
public current: Option<T> = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Option 😍

@kevinkucharczyk kevinkucharczyk merged commit cf1153a into main Sep 28, 2023
2 checks passed
@kevinkucharczyk kevinkucharczyk deleted the context-via-glimmer-vm branch September 28, 2023 04:52
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