This repository has been archived by the owner. It is now read-only.

Parser should account for multiple documents and throw error when ID already registered. #4

Closed
mwistrand opened this Issue Nov 8, 2015 · 1 comment

Comments

Projects
None yet
2 participants
@mwistrand
Contributor

mwistrand commented Nov 8, 2015

Note that the following concerns https://github.com/dojo/parser/blob/master/src/parser.ts#L77-L79. Considering that the parser accounts for multiple documents simultaneously, the idMap should as well, especially since node IDs are only supposed to be unique on a per-document basis. As for IDs that have already been registered within a single document, I'm inclined to believe an error should be thrown. Otherwise users might get into trouble without an immediate indication of the problem. For example, consider the following scenario:

<div id="root">
    <p id="child"></p>
</div>
const root = document.getElementById('root');
watch({
    root: root
});

const oldChild = document.getElementById('child');
const newChild = document.createElement('div');
newChild.id = 'child';

root.insertBefore(newChild, oldChild);
root.removeChild(oldChild);

Since the new node is added before the old node is removed, instantiateParserObject will try to add the new instance to the idMap before the old one has been removed. As a result, byNode(newChild) will work, but byId(newChild.id) will not.

@kitsonk kitsonk added the discussion label Nov 13, 2015

@kitsonk

This comment has been minimized.

Show comment
Hide comment
@kitsonk

kitsonk Nov 13, 2015

Member

Yes, I am not 100% sure I had multi-document handling done fully as well as what to do about duplicate IDs... glad you picked up on it.

Member

kitsonk commented Nov 13, 2015

Yes, I am not 100% sure I had multi-document handling done fully as well as what to do about duplicate IDs... glad you picked up on it.

@kitsonk kitsonk added this to the alpha.1 milestone Mar 11, 2016

@kitsonk kitsonk modified the milestones: 2016.05, alpha.1 Apr 8, 2016

@kitsonk kitsonk closed this in #6 Apr 8, 2016

kitsonk added a commit that referenced this issue Apr 8, 2016

Add multiple-document support
* Add multiple-document support.
* Allow objects to be registered to multiple documents.
* Throw an error when objects are registered with existing IDs.
* Style updates.

1. Extract ID map into a common interface.
2. Fix lint errors.

* Update `byId` signature.
* Allow document to be specified as first parameter to `byId`.
* Make `IdMap` interface private.

Closes #4

@kitsonk kitsonk modified the milestones: 2016.04, 2016.05 Apr 12, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.