Bring @glimmer/component into ember repo #20751
Conversation
ef4
force-pushed
the
glimmer-component
branch
from
9bcde59
to
7d6699b
Compare
The glimmerjs/glimmer.js monorepo is littered with "standalone Glimmer.js" functionality that we don't use and that is not being maintained. But it also contains two packages that are critical to ember: `@glimmer/component` and `@glimmer/tracking`. I want to bring both of those packages into here instead, while jettisoning their extra complexity that exists only to make them portable to the "standalone Glimmer.js" use case. This PR brings over @glimmer/component, and simplifies it by: - dropping support for Ember < 3.13. - converting to a v2 addon This also adds end-to-end test coverage exercising an interactive glimmer component. valid v2 addon expanding end-to-end test coverage to exercise glimmer component and glimmer tracking
ef4
force-pushed
the
glimmer-component
branch
from
7d6699b
to
816c17c
Compare
| @@ -3,6 +3,7 @@ | |||
| "version": "0.0.0", | |||
| "description": "Internal APIs shared by Ember packages. Warning: this package does not follow SemVer and is subject to break at any time!", | |||
| "type": "module", | |||
| "private": true, | |||
There was a problem hiding this comment.
I verified that all the other package.jsons were also private.
Surprised this wasn't caught before -- but also not surprised -- because we're not using release-plan (or anything to enforce package.json consistency)
There was a problem hiding this comment.
I think for now, we could very well set up release plan for just @glimmer/component if we wanted.
| ] | ||
| } | ||
| }, | ||
| "ember-addon": { |
There was a problem hiding this comment.
since glimmer/component doesn't depend on any embery things, does it need to be an addon at all?
There was a problem hiding this comment.
I thought that too, but it imports from @ember/destroyable, @ember/component, and @ember/runloop. Those only work in an addon.
| export type Args<S> = ExpandSignature<S>['Args']['Named']; | ||
|
|
||
| /** | ||
| * The `Component` class defines an encapsulated UI element that is rendered to |
There was a problem hiding this comment.
do these docs show up on the api docs?
There was a problem hiding this comment.
Good question.
I see there are actually comment-only files that already existed to hold the @glimmer/component docs in this repo. Those can be merged with the real files now.
| @@ -0,0 +1,286 @@ | |||
| import { DEBUG } from '@glimmer/env'; | |||
|
|
|||
| const DESTROYING = new WeakMap<GlimmerComponent<object>, boolean>(); | |||
There was a problem hiding this comment.
I know this is copied from the the old repo -- but I feel like we could use the destroyable primitives instead (which i'm more than happy to PR after this (#20751) lands)
export {
assertDestroyablesDestroyed,
associateDestroyableChild,
destroy,
enableDestroyableTracking,
isDestroying,
isDestroyed,
} from '@glimmer/destroyable';
import { registerDestructor } from '@glimmer/destroyable';https://github.com/glimmerjs/glimmer-vm/blob/main/packages/%40glimmer/destroyable/index.ts#L204
There was a problem hiding this comment.
Yeah that makes sense as a followup refactor.
|
|
||
| setDestroying(component); | ||
|
|
||
| schedule('actions', component, component.willDestroy); |
There was a problem hiding this comment.
where is this type of destruction scheduled in specific queues? does all destruction work like this?
I don't know that I've seen this outside of @glimmer/component
destroy in @glimmer/destroyable does have scheduling behavior as well 🤔 https://github.com/glimmerjs/glimmer-vm/blob/main/packages/%40glimmer/destroyable/index.ts#L153 -- but it's different.
There was a problem hiding this comment.
I think the original intent of this code is just that the callback is not synchronous. My guess is that this could switch to @ember/destroyable and people wouldn't a meaningful difference in timing.
|
Even if we add release-plan just for @glimmer/component, we need to consider ember.js publishing. Right now, ember.js publishing runs on every tag. Changelogs are generated with bin/changelog.js and depend upon all PRs being just for that package (and follows references in commits via I think it can be done but there's a lot to update. |
|
I think the smallest next action to move this ahead would be to decide on a tagging strategy that would allowed tagged releases for I created #20753 as one possibility. |
|
When we bump ember-source in the app blueprint to the version containing this change, it would also be a good time to bump |
|
minor - looks we missed to bring I guess we could just copy-paste the existing one? e.g. |
The glimmerjs/glimmer.js monorepo is littered with "standalone Glimmer.js" functionality that we don't use and that is not being maintained. But it also contains two packages that are critical to ember:
@glimmer/componentand@glimmer/tracking.I want to bring both of those packages into here instead, while jettisoning their extra complexity that exists only to make them portable to the "standalone Glimmer.js" use case.
This PR brings over @glimmer/component, and simplifies it by:
It also adds end-to-end test coverage that exercises @glimmer/component and @glimmer/tracking in both classic and embroider apps.
After we decide to land this, a followup will be to add release automation for
@glimmer/component.