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

[FEATURE | BREAKING] Change semantics of in-element to match emberjs/rfcs#287 #918

Merged
merged 1 commit into from
Mar 29, 2019

Conversation

chadhietala
Copy link
Member

@chadhietala chadhietala commented Feb 11, 2019

This implements the clearing semantics of in-element as it pertains to emberjs/rfcs#287. Instead of appending we need to clear the contents of the destination element unless insertBefore was explicitly set to null.

This is required work for emberjs/rfcs#418

packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts Outdated Show resolved Hide resolved
packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts Outdated Show resolved Hide resolved
packages/@glimmer/runtime/lib/vm/rehydrate-builder.ts Outdated Show resolved Hide resolved
packages/@glimmer/node/lib/serialize-builder.ts Outdated Show resolved Hide resolved
packages/@glimmer/node/lib/serialize-builder.ts Outdated Show resolved Hide resolved
packages/@glimmer/node/lib/serialize-builder.ts Outdated Show resolved Hide resolved
packages/@glimmer/runtime/lib/vm/rehydrate-builder.ts Outdated Show resolved Hide resolved
packages/@glimmer/runtime/lib/vm/element-builder.ts Outdated Show resolved Hide resolved
@chancancode
Copy link
Contributor

chancancode commented Mar 14, 2019

I think I have a few recommendations:

  • Need tests for the "replace" case (i.e. when insertBefore is not specified).
  • Option<null> doesn't make sense, since it just expands into null | null.
  • I think it would make sense to keep the internal pushRemoteElement API taking an element and a nextSibling. This is basically the "DOM cursor" concept that is used throughout the element builder code. Changing this would make it inconsistent with e.g. pushElement.
  • Instead, in the clearing case, I think it would make more sense for the caller to clear the target element (may require adding APIs to the element build or somewhere else, not sure), then call pushRemoteElement with the cleared element and null nextSibling.

- Remove nextSibling in favor of insertBefore
- Change semantics of insertBefore to always clear the remote element before inserting
@chadhietala
Copy link
Member Author

I added the test. We can do a refactor in the future to break apart the clearing out the target. I had some questions about the Enum map for operations since it has dups in it.

@chadhietala chadhietala merged commit 77dcea0 into master Mar 29, 2019
@chadhietala chadhietala deleted the impl-in-element-api branch March 29, 2019 19:15
chiragpat added a commit to chiragpat/glimmer-vm that referenced this pull request Apr 10, 2019
- This is a slight modification of the changes introduced in PR glimmerjs#918, to continue to allow passing non null values to the insertBefore param of the in-element helper
chiragpat added a commit to chiragpat/glimmer-vm that referenced this pull request Apr 10, 2019
- This is a slight modification of the changes introduced in PR glimmerjs#918, to continue to allow passing non null values to the insertBefore param of the in-element helper
chiragpat added a commit to chiragpat/glimmer-vm that referenced this pull request Apr 10, 2019
- This is a slight modification of the changes introduced in PR glimmerjs#918, to continue to allow passing non null values to the insertBefore param of the in-element helper
chiragpat added a commit to chiragpat/glimmer-vm that referenced this pull request Apr 10, 2019
- This is a slight modification of the changes introduced in PR glimmerjs#918, to continue to allow passing non null values to the insertBefore param of the in-element helper
chiragpat added a commit to chiragpat/glimmer-vm that referenced this pull request Apr 10, 2019
- This is a slight modification of the changes introduced in PR glimmerjs#918, to continue to allow passing non null values to the insertBefore param of the in-element helper
@rwjblue rwjblue changed the title [FEATURE | BREAKING] Change semantics of in-element [FEATURE | BREAKING] Change semantics of in-element to match emberjs/rfcs#287 Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants