[FEATURE] Update Glimmer-VM to 0.45.0 #18621
Conversation
|
thanks for handling this! |
|
Closes #18182. |
|
Shows a minor regression, likely due to new code that has not be optimized just yet. |
|
I'm working through the review here, I pinged @krisselden and @chancancode for reviews as well. |
| unwrapTemplate(localLayout).referrer.moduleName === | ||
| unwrapTemplate(globalLayout).referrer.moduleName |
There was a problem hiding this comment.
@pzuraq - Can you add a comment (and maybe an issue) to remove this whole section for MU support?
| ['nested-item', 'didInsertElement'], | ||
| ['nested-item', 'didRender'], | ||
| ['no-items', 'didInsertElement'], | ||
| ['no-items', 'didRender'], |
There was a problem hiding this comment.
This seems like it may be a semi fundamental shift, and we should 1) make sure that we have to do it and 2) ensure that the behavioral change is properly documented and understood.
Can you explain why we change this, how we consider this a non-breaking change, etc?
There was a problem hiding this comment.
Discussed this offline, the change was caused by removing the custom curly-component destruction queue that existed on the environment before. This means that all components now call their didDestroy hooks after their didCreate and didUpdate hooks. Before, there was divergent behavior between curly components and other types of components here.
I believe the conclusion was that this change is relatively low risk, since the timing of didDestroy style hooks has always been async/later and generally isn't relied as much as willDestroy style hooks. If we encounter issues later due to this change, we can either update Glimmer-VM directly to move the position of the queue, or we can add back the custom queue specifically for curly components.
There was a problem hiding this comment.
Thank you for summarizing our conversation on this! We will just need to monitor for any issues reported (worse case scenario we bring back a custom queue that we flush like before, but we'd much prefer to avoid that).
|
Notes for followup:
|
|
@pzuraq Curious why would Named Blocks be disabled? That's one of the super exciting features that many of us are waiting to get. |
this is something I've been excited about, too :D |
|
We agreed during the development process that we would land this PR first and get a release cycle out before we enable any new features. That way we can focus on fixing any bugs and perf issues related to the upgrade itself, and enabled new features once the dust has settled. This is pretty standard operating procedure for major upgrades like this one. |
|
Interesting. Any chance we can get Named Blocks under a canary feature flag? That way we can start using/testing it. |
|
Yes, that is exactly the plan. The things that @pzuraq mentioned in the comment above will be disabled, then we can bring each of them back behind feature flags. |
|
@pzuraq thank you for amazing work! |
|
@pzuraq Awesome! 🎉 |
Closes #18182