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

[experiment]: Demo app for glimmer-next renderer #20711

Draft
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

lifeart
Copy link
Contributor

@lifeart lifeart commented Jun 29, 2024

This is experiment to replace @glimmer/* rendering and reactivity stack with lifeart/glimmer-next

Demo Application setup based on source from lifeart/demo-ember-vite

Check:

cd packages/demo/
rm -rf ./node_modules/.vite
pnpm run dev 

Routes:
http://localhost:5173/profile
http://localhost:5173/

Working (somehow):

  • tracked
  • alias
  • internal component manager
  • LinkTo
  • Input
  • routing
  • {{outlet}}
  • queryParams

Not working:

  • Backburner driven rendering
  • MutableCells
  • Custom outlets
Screen.Recording.2024-07-01.at.16.13.54.mov

Copy link
Sponsor Contributor

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

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

It's fun to see some exploration happening here! 💪 🎉

packages/ember/index.ts Outdated Show resolved Hide resolved
@@ -1,7 +1,10 @@
import type { InternalOwner } from '@ember/-internals/owner';
import type { Helper, HelperDefinitionState } from '@glimmer/interfaces';
import { setInternalHelperManager } from '@glimmer/manager';
// import { setInternalHelperManager } from '@glimmer/manager';
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

why comment this out?

If we need to re-define the implementation, I think that could happen in this PR (like, if it's copying from glimmer-next, for example)

@@ -4,6 +4,7 @@ import type { InternalOwner } from '@ember/-internals/owner';
import { getOwner } from '@ember/-internals/owner';
import { guidFor } from '@ember/-internals/utils';
import { getViewElement, getViewId } from '@ember/-internals/views';
import { renderComponent } from '@lifeart/gxt';
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

we should pull this in to the repo (easier maintenance!)

@@ -0,0 +1,58 @@
import { Component } from '@lifeart/gxt';
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

why not @glimmer/component (not the real glimmer component, but re-defined in this PR, using glimmer-next apis?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

need to do some copy-paste from non-released branch - lifeart/glimmer-next#25

packages/@ember/routing/route.ts Outdated Show resolved Hide resolved
+
@glimmer/manager
Copy link
Sponsor Contributor

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

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

image

hopefully some @glimmer/* packages can be added here?
(for compat, we'd need to re-define @glimmer/component, @glimmer/tracking, etc as their own packages)

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

why not use ember-resolver?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NullVoxPopuli it's basically copy pasta, without side deps (glimemer-debug, etc).

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

you could provide @glimmer/debug in this PR :)

@@ -0,0 +1,14 @@
export function unwrapTemplate(tpl) {
return {
asLayout(){
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

I could be misremembering the purpose of this...
But, I don't think layouts need support as template resolving outside of routes' templates is deprecated for removal in v6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for now it's workaround related to existing template handing logic (from glimmer-vm).
compiled glimmer-vm template has special 'shape' and it's hardcoded in ember - we just providing compatible wrapper for it.

}
);
{{on 'click' this.click}} >{{yield}}</a>`;
}
Copy link
Contributor

@ef4 ef4 Jul 5, 2024

Choose a reason for hiding this comment

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

What is the motivation for needing to rewrite these?

You could reimplement the meaning of precompileTemplate to be whatever you need it to be, and then you wouldn't need to touch this code and you would have a much greater chance of actually achieving upgrade compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ef4 this is just for experimentation purposes, I agree - we could keep same api as-is here (compiler need to be extended a bit)


export default class ApplicationTemplate extends Component {
<template>
<header class="bg-blue-600 p-4">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: move to glimmer component

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.

3 participants