fix(compiler): Compiler errors for missing keys in iterator #138
Conversation
| @@ -255,6 +254,11 @@ export function i(iterable: Iterable<any>, factory: (value: any, index: number, | |||
| let next = iterator.next(); | |||
| let j = 0; | |||
| let { value, done: last } = next; | |||
| if (process.env.NODE_ENV !== 'production') { | |||
| // var is intentional here, function level scoping is required. | |||
| var keyMap = {}; | |||
There was a problem hiding this comment.
I couldn't think of a better way to hoist a map of used keys that will get populated in the while loop. Suggestions happily taken :)
There was a problem hiding this comment.
Thinking about this a little bit more, I think this will still get minified if I use let outside of the prod check
| @@ -271,9 +275,14 @@ export function i(iterable: Iterable<any>, factory: (value: any, index: number, | |||
| if (process.env.NODE_ENV !== 'production') { | |||
| const vnodes = isArray(vnode) ? vnode : [vnode]; | |||
| vnodes.forEach((childVnode) => { | |||
| if (!isNull(childVnode) && isObject(childVnode) && !isUndefined(childVnode.sel) && childVnode.sel.indexOf('-') > 0 && isUndefined(childVnode.key)) { | |||
There was a problem hiding this comment.
I don't think we want the childVnode.sel.indexOf('-') check here. This is valid for all elements, not just custom
| objToKeyMap.set(unwrapped, objKey); | ||
| } | ||
| return compilerKey + ':' + objKey; | ||
| throw new Error(`Invalid key value ${obj}. Key must be a string or number.`); |
There was a problem hiding this comment.
Opted for an Error instead of assert because I believe we will want this in production.
There was a problem hiding this comment.
you will have to do toString() around this, otherwise it might throw that it can't be stringify.
There was a problem hiding this comment.
this error is only incomplete, we should at least tell them what VM is this, but in prod we don't have toString on VM.
my recommendation is to keep it as an assert until the next refactor of the errors.
| @@ -122,23 +122,3 @@ export function getForEachParent(element: IRElement): IRElement | null { | |||
|
|
|||
| return null; | |||
| } | |||
|
|
|||
| export function keyExpression(element: IRElement) { | |||
There was a problem hiding this comment.
We no longer need to climb the iterator tree to magically bind the iterator value to our key. Instead, if the user has not defined their own key, we throw.
| @@ -110,7 +111,7 @@ export default function parse(source: string, state: State): { | |||
| applyHandlers(element); | |||
| applyComponent(element); | |||
| applySlot(element); | |||
| applyKey(element); | |||
| applyKey(element, elementNode.__location); | |||
There was a problem hiding this comment.
@pmdartus __location seems like a bad idea to use here?
There was a problem hiding this comment.
The element has access to the original node from the __original property.
No need to pass the location there.
There was a problem hiding this comment.
It looks like __original is an instance of HTMLElement, not parse5.AST.Default.Element
| @@ -373,10 +374,9 @@ export default function parse(source: string, state: State): { | |||
| } | |||
|
|
|||
| element.forKey = keyAttribute.value; | |||
| } else if ((getIteratorParent(element) || getForEachParent(element)) && element.tag !== 'template') { | |||
There was a problem hiding this comment.
This can possibly be abstracted out to isIteratorElement that returns true is element will be iterated over.
| @@ -255,6 +254,11 @@ export function i(iterable: Iterable<any>, factory: (value: any, index: number, | |||
| let next = iterator.next(); | |||
| let j = 0; | |||
| let { value, done: last } = next; | |||
| let keyMap; | |||
| if (process.env.NODE_ENV !== 'production') { | |||
| keyMap = {}; | |||
There was a problem hiding this comment.
create(null) to avoid surprises.
| // TODO - it'd be nice to log the owner component rather than the iteration children | ||
| assert.logWarning(`Missing "key" attribute in iteration with child "<${childVnode.sel}>", index ${i}. Instead set a unique "key" attribute value on all iteration children so internal state can be preserved during rehydration.`); | ||
| if (!isNull(childVnode) && isObject(childVnode) && !isUndefined(childVnode.sel)) { | ||
| if (isUndefined(childVnode.key)) { |
There was a problem hiding this comment.
probably better to just test for typeof to be number or string
Benchmark comparisonBase commit:
|
|
I have some usability concern with this change, especially with external customers. Even if React and Vue offer the capability to add keys but doesn't require it. I would expect that if we throw at compile time, we will run into the same issue than the React community is running into where people use the array index has a key. If we really want to proceed with this change, we should also ensure that the |
Benchmark comparisonBase commit:
|
We should start with the more restrictive, and eventually relax if possible, as we always do!
I think this is a good suggestion, and in the compiler, we can know for sure if the identifier is the one defined in the iteration as the index, and throw for now. |
|
guys, this should be part of the next release, otherwise people will face issues that they can't explain when no keys are provided. |
|
@caridy do we still want to implement throwing when the user uses index? |
|
@davidturissini yes, I think we should do that to avoid any confusion. |
| keyMap[key] = 1; | ||
| } else { | ||
| // TODO - it'd be nice to log the owner component rather than the iteration children | ||
| assert.logWarning(`Missing "key" attribute in iteration with child "<${childVnode.sel}>", index ${i}. Instead set a unique "key" attribute value on all iteration children so internal state can be preserved during rehydration.`); |
There was a problem hiding this comment.
Instead set a unique "key" - 'instead' word is not needed here since there was no alternative. We can just start with 'Set a unique key ...'
| @@ -3,7 +3,7 @@ | |||
| "length": 11, | |||
| "level": "error", | |||
| "message": "Key attribute value should be an expression", | |||
| "start": 83 | |||
There was a problem hiding this comment.
that is a big gap 83 vs 19
| if (keyAttribute.type !== IRAttributeType.Expression) { | ||
| return warnAt(`Key attribute value should be an expression`, keyAttribute.location); | ||
| } | ||
|
|
||
| if (isForOfChild(element)) { | ||
| if (keyAttribute.value.property.name === 'index') { |
There was a problem hiding this comment.
@pmdartus There is a type error here and it's difficult to trace exactly what keyAttribute should be. Typescript doesn't like me looking at property key on value. Any tips?
Benchmark comparisonBase commit:
|
Benchmark comparisonBase commit:
|
Details
Not including a key in iterators(for:each and iterator:foo) will now produce a compile time error. Furthermore, using duplicate keys inside of an iteration will now warn the user as this is most likely a mistake and will result in incorrect markup.
Does this PR introduce a breaking change?
If yes, please describe the impact and migration path for existing applications:
Please check if your PR fulfills the following requirements:
We are currently auto-generating keys in iterators via a weak map in
api.k. This is proving to be problematic so we have decided to change direction and require devs to supply a key value themselves. This will break downstream components.Fixes #136