[REFACTOR] Upstreams basic manager infrastructure and types #1197

pzuraq · 2020-11-12T22:20:48Z

Upstreams the basic manager infrastructure and types to Ember. This will
be followed by upstreaming the respective managers for each type, along
with some of the base managers.

Breaking Changes:

Renamed ComponentManager -> InternalCompnoentManager
Renamed ModifierManager -> InternalModifierManager
Upstreamed manager types from Ember
Upstreamed setManager and getManager functions from Ember
Upstreamed buildCapabilities from Ember

Ember integration: emberjs/ember.js#19267

rwjblue

Some open questions:

We should migrate things that can use the public managers to using them (e.g. benchmark, integration-tests for things like "glimmerishcomponent", etc). Can you make an issue with a TODO so that we don't loose track? Refactor test and benchmark envs to use public managers #1201
Can you link to the Ember integration PR in the description?
Have you reviewed the work to integration in Glimmer.js? Is it roughly the same / very similar to the work for Ember integration? [FEAT] Upgrade to Glimmer VM v0.65.0 glimmer.js#317
Can you add tests to confirm that buildCapabilities is required for each of the types (we have had a number of regressions on this over in the Ember side, and it would be good to ensure this is under test)?
Can you add tests to confirm that .capabilities is the result of calling buildCapabilities?
We need to address the quality of the error messages from buildCapabilities. Either to make it host provided or unify the import path. Can you make an issue to track that and add an inline link above each of the error messages that say @ember/*? Make error messages meaningful in both Ember and Glimmer #1200

Future looking questions:

We have to figure out where the API versions come from now that the code itself is not colocated in Ember. It is quite difficult to know what version of Ember a given WIP in glimmer-vm will target, so choosing the version for new capabilities versions might be pretty hard.
Now that the managers (and the user-facing manager APIs) are moved into glimmer-vm, do we still want the delegate system? Naively (to me) it seems like that is just wasted effort (aliasing hooks from one to the other, instantiating extra objects, etc).

packages/@glimmer/benchmark-env/src/benchmark/create-registry.ts

rwjblue · 2020-11-13T14:22:22Z

packages/@glimmer/interfaces/lib/managers/component.d.ts

+  '3.4': {
+    asyncLifecycleCallbacks?: boolean;
+    destructor?: boolean;
+  };
+
+  '3.13': {
+    asyncLifecycleCallbacks?: boolean;
+    destructor?: boolean;
+    updateHook?: boolean;
+  };


I definitely have no objection here (and this isn't a blocking concern), but we need to figure out how we are going to pick these numbers in the future.

Agreed, wholeheartedly. I think honestly just versioning each API independently is probably a decent solution, as long as we document them in the API docs. It means users will have to do a bit of extra looking-up to figure out what capabilities are available (and supported), but that's a given with this APIs anyways.

rwjblue · 2020-11-13T14:24:21Z

packages/@glimmer/interfaces/lib/managers/internal/component.d.ts

+/**
+ * Describes the capabilities of a particular component. The capabilities are
+ * provided to the Glimmer compiler and VM via the ComponentDefinition, which
+ * includes a ComponentCapabilities record.
+ *
+ * Certain features in the VM come with some overhead, so the compiler and
+ * runtime use this information to skip unnecessary work for component types
+ * that don't need it.
+ *
+ * For example, a component that is template-only (i.e., it does not have an
+ * associated JavaScript class to instantiate) can skip invoking component
+ * manager hooks related to lifecycle events by setting the `elementHook`
+ * capability to `false`.
+ */


Nice ❤️

packages/@glimmer/runtime/lib/component/manager.ts

rwjblue · 2020-11-13T14:39:24Z

packages/@glimmer/runtime/lib/managers/index.ts

+      !FROM_CAPABILITIES!.has(manager.capabilities)
+    ) {
+      throw new Error(
+        `Custom modifier managers must have a \`capabilities\` property that is the result of calling the \`capabilities('3.13' | '3.22')\` (imported via \`import { capabilities } from '@ember/modifier';\`). Received: \`${JSON.stringify(


Assuming that Glimmer.js will also leverage this infrastructure (I assume it has to), we need to allow this suggested string to be "host dependent". Can we have this configured by the host instead?

Obviously, we should also work to make the value the same (so that both Ember and Glimmer.js apps import from the same place), but we'll need an RFC for that...

IMHO, making the error message less useful is not a valid choice here, I think if we have to land this as @ember/modifier in the string for now that is "fine" (by some definition 😝 ) but we should make an issue (and an inline comment pointing to that issue) to come back and clean this up (either by allowing the host to customize the value, or by doing the RFC in Ember land to allow the import to be from an @glimmer namespace).

rwjblue · 2020-11-13T14:41:35Z

packages/@glimmer/runtime/lib/managers/index.ts

+
+    if (DEBUG && !isInternalHelper(manager) && !FROM_CAPABILITIES!.has(manager.capabilities)) {
+      throw new Error(
+        `Custom helper managers must have a \`capabilities\` property that is the result of calling the \`capabilities('3.23')\` (imported via \`import { capabilities } from '@ember/helper';\`). Received: \`${JSON.stringify(


Same as above RE: the error message

rwjblue · 2020-11-13T14:41:52Z

packages/@glimmer/runtime/lib/managers/index.ts

+      !FROM_CAPABILITIES!.has(manager.capabilities)
+    ) {
+      throw new Error(
+        `Custom component managers must have a \`capabilities\` property that is the result of calling the \`capabilities('3.4' | '3.13')\` (imported via \`import { capabilities } from '@ember/component';\`). Received: \`${JSON.stringify(


Same RE: error message

pzuraq · 2020-11-13T17:49:49Z

Can you add tests to confirm that buildCapabilities is required for each of the types (we have had a number of regressions on this over in the Ember side, and it would be good to ensure this is under test)? This would mean both that .capabilities must be on your manager instance and that it is the result of calling buildCapabilities..

We do have tests for these: https://github.com/glimmerjs/glimmer-vm/pull/1197/files#diff-1e72e3bda8309fc230d43679cb9d9112b08cc1b676f6a7899f1907b7984f094eR224

rwjblue · 2020-11-13T18:21:43Z

Can you add tests to confirm that buildCapabilities is required for each of the types (we have had a number of regressions on this over in the Ember side, and it would be good to ensure this is under test)? This would mean both that .capabilities must be on your manager instance and that it is the result of calling buildCapabilities..

We do have tests for these: #1197 (files)

Hmm, that only confirms that you have a capabilities property, not that it is the result of calling the internal branded thing.

Upstreams the basic manager infrastructure and types to Ember. This will be followed by upstreaming the respective managers for each type, along with some of the base managers. Breaking Changes: - Renamed `ComponentManager` -> `InternalCompnoentManager` - Renamed `ModifierManager` -> `InternalModifierManager` - Upstreamed manager types from Ember - Upstreamed `setManager` and `getManager` functions from Ember - Upstreamed `buildCapabilities` from Ember

pzuraq · 2020-11-13T18:39:15Z

Good point! Updated and added another set of tests specifically for a non-built value, not a missing value.

pzuraq · 2020-11-13T20:07:31Z

Re: Updating on the Glimmer side, the updates are relatively minimal. This is the PR for it: glimmerjs/glimmer.js#317

rwjblue requested changes Nov 13, 2020

View reviewed changes

pzuraq force-pushed the upstream/manager-infrastructure branch from d1e1ccc to bff1273 Compare November 13, 2020 17:54

pzuraq force-pushed the upstream/manager-infrastructure branch from bff1273 to 9e7289f Compare November 13, 2020 18:35

pzuraq force-pushed the upstream/manager-infrastructure branch from 9e7289f to 5f2d9b0 Compare November 13, 2020 18:38

rwjblue approved these changes Nov 13, 2020

View reviewed changes

rwjblue merged commit f5ea79b into master Nov 13, 2020

rwjblue deleted the upstream/manager-infrastructure branch November 13, 2020 20:10

rwjblue added the breaking label Nov 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REFACTOR] Upstreams basic manager infrastructure and types #1197

[REFACTOR] Upstreams basic manager infrastructure and types #1197

pzuraq commented Nov 12, 2020 •

edited

Loading

rwjblue left a comment •

edited

Loading

rwjblue Nov 13, 2020

pzuraq Nov 13, 2020

rwjblue Nov 13, 2020

rwjblue Nov 13, 2020

rwjblue Nov 13, 2020

rwjblue Nov 13, 2020

rwjblue Nov 13, 2020

pzuraq commented Nov 13, 2020

rwjblue commented Nov 13, 2020

pzuraq commented Nov 13, 2020

pzuraq commented Nov 13, 2020

[REFACTOR] Upstreams basic manager infrastructure and types #1197

[REFACTOR] Upstreams basic manager infrastructure and types #1197

Conversation

pzuraq commented Nov 12, 2020 • edited Loading

rwjblue left a comment • edited Loading

Choose a reason for hiding this comment

rwjblue Nov 13, 2020

Choose a reason for hiding this comment

pzuraq Nov 13, 2020

Choose a reason for hiding this comment

rwjblue Nov 13, 2020

Choose a reason for hiding this comment

rwjblue Nov 13, 2020

Choose a reason for hiding this comment

rwjblue Nov 13, 2020

Choose a reason for hiding this comment

rwjblue Nov 13, 2020

Choose a reason for hiding this comment

rwjblue Nov 13, 2020

Choose a reason for hiding this comment

pzuraq commented Nov 13, 2020

rwjblue commented Nov 13, 2020

pzuraq commented Nov 13, 2020

pzuraq commented Nov 13, 2020

pzuraq commented Nov 12, 2020 •

edited

Loading

rwjblue left a comment •

edited

Loading