-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Bring @glimmer/component into ember repo #20751
Conversation
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do these docs show up on the api docs?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that makes sense as a followup refactor.
|
||
setDestroying(component); | ||
|
||
schedule('actions', component, component.willDestroy); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/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:
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
.