[DO NOT MERGE] Glimmer 2 Final Countdown #13316
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chancancode
changed the title
chadhietala
force-pushed
the
final-countdown
branch
from
7a06196
to
e877cf7
Compare
|  | ||
|
|
||
| # Macro Tasks | ||
| - [ ] Complete all test migration described in #13127 |
There was a problem hiding this comment.
- Complete all test migration described in [Pls send halp!] The Ultimate Glimmer 2 Test Porting Guide #13127
|
|
||
| ### Class Helper | ||
| - [ ] Should compute when params change: [Failing test](https://github.com/emberjs/ember.js/blob/b418b95ea3d31e3a3656906e6ad784422f648c31/packages/ember-glimmer/tests/integration/helpers/custom-helper-test.js#L147) | ||
| - [ ] Not usable in a block: [Failing test](https://github.com/emberjs/ember.js/blob/b418b95ea3d31e3a3656906e6ad784422f648c31/packages/ember-glimmer/tests/integration/helpers/custom-helper-test.js#L295) |
There was a problem hiding this comment.
- Not usable in a block: Failing test
chadhietala
force-pushed
the
final-countdown
branch
from
e877cf7
to
1560357
Compare
| - [ ] Implement `{{mut}}` | ||
| - [ ] Implement `{{textarea}}`: [Failing test 1](https://github.com/emberjs/ember.js/blob/b418b95ea3d31e3a3656906e6ad784422f648c31/packages/ember-glimmer/tests/integration/helpers/text-area-test.js#L15) [Failing test 2](https://github.com/emberjs/ember.js/blob/b418b95ea3d31e3a3656906e6ad784422f648c31/packages/ember-glimmer/tests/integration/helpers/text-area-test.js#L23) [Failing test 3](https://github.com/emberjs/ember.js/blob/b418b95ea3d31e3a3656906e6ad784422f648c31/packages/ember-glimmer/tests/integration/helpers/text-area-test.js#L36) | ||
| - [ ] Implement `{{each-in}}` [Failing tests](packages/ember-glimmer/tests/integration/syntax/each-in-test.js#L23) | ||
| - [ ] Implement `{{action}}` |
There was a problem hiding this comment.
- Implement
{{action}}
|
There are a few |
chadhietala
force-pushed
the
final-countdown
branch
2 times, most recently
from
57262af
to
d7682b6
Compare
|
I will implement |
chadhietala
force-pushed
the
final-countdown
branch
4 times, most recently
from
c0564c9
to
a6bc888
Compare
| - [ ] Implement remaining life-cycle hooks | ||
| - [ ] Implement custom Ids for curly component invocations: [Failing test](https://github.com/emberjs/ember.js/blob/b418b95ea3d31e3a3656906e6ad784422f648c31/packages/ember-glimmer/tests/integration/components/curly-components-test.js#L26) | ||
| - [x] Implement triple curiles #13329 | ||
| - [x] Implement `htmlSafe` #13329 |
There was a problem hiding this comment.
htmlSafe is not actually supported in double-curlies from what I can tell. The PR mentioned here only fixes the tests to make it possible to compare HTMLBars and Glimmer2 behavior side-by-side. The feature is still missing (and I'm trying to implement it).
There was a problem hiding this comment.
You should look here: https://github.com/tildeio/glimmer/blob/master/packages/glimmer-runtime/lib/compiled/opcodes/content.ts
- The
AppendOpcodeis responsible for inserting the content during "initial render" (but this also applies to things like an{{#if}}changing from true to false, thus tearing down the block and rendering a different one). It needs to be updated to handle both safe and non-safe strings. - The
AppendOpcodeemits anUpdateAppendOpcodethat is responsible for keeping the curlies up-to-date when the content changes. It needs to remember which "mode" it was in previously and do the right thing. (Handling safe <-> non-safe strings, etc). - You can look at how the "Trusting" versions of opcodes, which are for triple-curlies. You can probably refactor things to share more code.
- Keep in mind the "const reference optimization" as you do this: Implement
ConstReferenceoptimization glimmerjs/glimmer-vm#109 - Future work: we also need to support appending DOM nodes into the curlies (helpers etc can return DOM nodes to be inserted). But we can tackle that separately.
- Ideally, we would like to minimize the "mode switching" stuff by emitting specialized opcodes, but I couldn't think of any cases where we can know these information ahead of time (other than the const case, which simply does not emit updating opcodes).
|
I'm investigating |
| ### Curly Component | ||
|
|
||
| - [x] Ensure `elementId` cannot change: [Failing test](https://github.com/emberjs/ember.js/blob/b418b95ea3d31e3a3656906e6ad784422f648c31/packages/ember-glimmer/tests/integration/components/curly-components-test.js#L48) | ||
| - [ ] Ensure `classBinding` without a condition applies correctly: [Failing Test](https://github.com/emberjs/ember.js/blob/b418b95ea3d31e3a3656906e6ad784422f648c31/packages/ember-glimmer/tests/integration/components/curly-components-test.js#L176) |
There was a problem hiding this comment.
I'm going to take a stab at this
EDIT: #13337
chadhietala
force-pushed
the
final-countdown
branch
from
a6bc888
to
aafb356
Compare
|
Bump. Just want to notify everyone in this thread on the way we are going to track things going forward. Please read the initial comment for an update. |
|
Closing in favor of #13644 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Update
Markdown file did not pan out. There was much confusion on what tasks were taken. So what is the plan? I have migrated all of the tasks and relevant information to a Trello board 😱 . No worries you do not need to signup for Trello or any of that nonsense. Please continue 🔒 ing things by just commenting on this PR along with the link to the trello board. Example:
I will then personally go move the card to "In Progress". Hopefully this is not too much burden and will actually result in clear direction of "what needs to be done".
Thanks everyone!
Board
https://trello.com/b/6C6i3eGV/glimmer-2
This is the 🔥 🔥 🔥 burn down 🔥 🔥 🔥 issue to ship Glimmer 2. While there are still in flight PRs, this is known state of the world at SHA b418b95.
We are experimenting on using a temporary Markdown instead of a regular checklist so that we can add line comments for each item. Do not merge this pull request.