marko-js / marko

A declarative, HTML-based language that makes building web apps fun
https://markojs.com/
MIT License
13.36k stars 643 forks source link

TypeError: fromEl is undefined in for loops #1381

Open reytech-dev opened 5 years ago

reytech-dev commented 5 years ago

Marko Version: 4.18.7

Details

I'm creating a select in a form. To fill it with options I'm giving in an array with objects. Each item object should create 1 option in the select. To create the option entries I'm using a for-loop which is looping through the array of items. For each item I'm adding key="index". When I create the select I'm facing the error "TypeError: fromEl is undefined" and the code execution stops.

Expected Behavior

I expected to have a select filled with several option entries.

Actual Behavior

I dont get any select returned.

Possible Fix

Apparently there is a problem with setting key="index" in a for-loop. I replaced key="index" with key=${index}-item and the error disappeared.

Additional Info ### Your Environment - Environment name and version (e.g. Chrome 39, node.js 5.4): - Firefox - node.js v11.15.0 - Operating System and version (desktop or mobile): Arch Linux, desktop ### Steps to Reproduce https://github.com/reytech-dev/markojs-poc Instruction to reproduce: https://github.com/reytech-dev/markojs-poc/blob/master/README.md Please let me know if you need any further information. ### Stack Trace TypeError: fromEl is undefined index.js:44 Marko 20 compareNodeNames morphChildren insertVirtualNodeBefore morphChildren morphComponent insertVirtualComponentBefore morphChildren insertVirtualNodeBefore morphChildren morphComponent insertVirtualComponentBefore morphChildren morphdom ___getNode getNode getEl replaceChildrenOf showList delegateEvent eventType
Sebring commented 5 years ago

By setting key="index" (making it a string with the quotes) you'll end up with all the items having the same key - "index" which most likely causes the list to fail.

By the way, key=index (no-quotes - same as ${index}) is the default value so there is no need to specify that. Better would be something like key=option.id in a for|option| of=options - loop

See Always set a key in https://markojs.com/docs/conditionals-and-lists/#lists

tcrowe commented 4 years ago

If you take this: https://github.com/marko-js/marko/blob/master/src/runtime/vdom/morphdom/index.js#L44

...and patch it like so:

function compareNodeNames(fromEl, toEl) {
    return fromEl !== undefined && toEl !== undefined && fromEl.___nodeName === toEl.___nodeName;
}

...then it's fine.

I reached this same error doing the following outside a loop and it's not clear if there is an appropriate place for key in this scenario.

<if(state.visible === true)>
  <${input.renderBody}/>
</if>
tcrowe commented 4 years ago

Even with the workaround patch there are some inconsistencies which I cannot understand yet. I built this repo to test some ideas. https://github.com/tcrowe/isomorphic-marko-template

@reytech-dev Maybe you can fork and reproduce the loop bug?

DylanPiercey commented 4 years ago

This is expected behavior although we should probably log a warning when it is encountered.

The key attribute needs to be unique for the given component. In this case you are giving each item in the loop the same key index.