feat(engine): abstraction for create/insert/remove/render to support CE #97
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
caridy
commented
Feb 13, 2018
| if (vm.isDirty) { | ||
| renderVM(vm); | ||
| } | ||
| renderVM(vm); |
There was a problem hiding this comment.
renderVM will take care of the check now.
caridy
commented
Feb 13, 2018
| const Ctor = getCtorByTagName(sel); | ||
| const { forceTagName } = Ctor as ComponentConstructor; | ||
| const tagName = isUndefined(forceTagName) ? sel : forceTagName; | ||
| // Create element with correct tagName | ||
| const element = document.createElement(tagName); | ||
| if (hasOwnProperty.call(element, ViewModelReflection)) { | ||
| return element; |
There was a problem hiding this comment.
if the element is registered somehow, then the VM will be attached to it, and we don't have to carry on the hacks to detection insertion and removal, as well as the initial creation of the VM because it was already created.
There was a problem hiding this comment.
how can this occur if we are creating brand new element right here?
caridy
commented
Feb 13, 2018
| } | ||
|
|
||
| export function appendVM(vm: VM) { | ||
| if (process.env.NODE_ENV !== 'production') { | ||
| assert.vm(vm); | ||
| assert.isTrue(vm.idx === 0, `${vm} is already inserted.`); | ||
| } | ||
| if (vm.idx !== 0) { |
There was a problem hiding this comment.
instead of an assertion, it is just a runtime check.
caridy
commented
Feb 13, 2018
| } | ||
| rehydrate(vm); | ||
| if (vm.isDirty) { |
There was a problem hiding this comment.
instead of an assertion, it is just a runtime check.
caridy
commented
Feb 13, 2018
| } | ||
| addInsertionIndex(vm); | ||
| } | ||
|
|
||
| export function removeVM(vm: VM) { | ||
| if (process.env.NODE_ENV !== 'production') { | ||
| assert.vm(vm); | ||
| assert.isTrue(vm.idx > 0, `${vm} is not inserted.`); | ||
| } | ||
| if (vm.idx === 0) { |
There was a problem hiding this comment.
instead of an assertion, it is just a runtime check.
caridy
commented
Feb 13, 2018
| @@ -128,6 +133,9 @@ export function createVM(tagName: string, elm: HTMLElement, cmpSlots?: Slotset) | |||
| if (process.env.NODE_ENV !== 'production') { | |||
| assert.invariant(elm instanceof HTMLElement, `VM creation requires a DOM element instead of ${elm}.`); | |||
| } | |||
| if (hasOwnProperty.call(elm, ViewModelReflection)) { | |||
There was a problem hiding this comment.
if the VM was already attached, somewhere else, there is nothing to do here.
Benchmark comparisonBase commit:
|
davidturissini
approved these changes
Feb 13, 2018
…render to support CE
caridy
force-pushed
the
caridy/custom-element-vs-raptor-element
branch
from
f8003ba
to
9d0379c
Compare
Benchmark comparisonBase commit:
|
caridy
changed the title
apapko
approved these changes
Feb 13, 2018
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.
Details
Instead of guarding in multiple places before carry-on any of these operations, we can just short-circuit on the spot. This will also help to support creation of registered elements.
Does this PR introduce a breaking change?