Allow reserved words to be used as keys

[robybully]

If a user uses the table in the lower left to edit tags and inserts "constructor" as key iD changes the key to "constructor_1". If it is changed to "constructor" again, iD changes it to "constructor_2". If it is again changed iD changes it again to "constructor_1" and so on...

Produced errors can be seen here: http://taginfo.openstreetmap.org//search?q=constructor

My browser is Firefox 45.0 with deactivated Adblock Plus on Windows 7.

[bhousel]

Discussed tangentially in #2896

The raw tag control is data bound to a JavaScript object. Other special keywords will do the same thing, e.g. you also can't enter tags like __proto__, __defineGetter__, hasOwnProperty, toString.

We should probably change this to use a d3.map.

[kepta]

@bhousel
Don't you think refactoring the code to use d3.map, would be an enormous task?
Or do you have a smart way to use d3.map?

I foolishly started replacing the codebase to use d3.map, but soon realized I should consult someone :D, before making any further changes.

[bhousel]

Yes, unfortunately this is an ugly issue, and it's something that needs an audit everywhere in the code that works with OSM tags. I guess we have 2 choices:

1. Replace tag objects with d3.map - or better yet, I think all the browsers we target support Native ES6 style Map - then replace everywhere we use dot notation with get/set/has/etc.
   ... or ...
2. Maybe use the Object.create(null) trick to create empty objects with no properties to conflict with OSM keys. But if we do this, we'd have to do such a significant code audit that we might as well just replace them with modern Maps.

@jfirebaugh any thoughts on what direction @kepta should take?

[jfirebaugh]

Is there an advantage to using native Map over Object.create(null)? I think we should do one or the other.

[bhousel]

I'm not sure.. Code I'm mostly concerned about is stuff like this in entity_editor.js:

tags = clean(_.extend({}, entity.tags, changed));

Switching to native Map seems safer than continuing to use Object to store OSM tags.

I can't think of any specific ways to break the existing tag handling code, because everything is treated as strings. But that doesn't mean it can't be done. (I have made a few simple attempts by creating objects in OSM with a __proto__ or hasOwnProperty key).

[kepta]

@bhousel @jfirebaugh Let us finalise on this one.

• native Map seems to be the choice
• Looking at compatibility chart it says Constructor argument: new Map(utterable) is not supported by IE 11 and safari. Would that be a problem?

[bhousel]

We can probably work around that and just not use the contructor argument.

detailed all browsers compatibility: http://kangax.github.io/compat-table/es6/#test-Map

I'm a little more worried about support on mobile. Even though iD doesn't really support mobile, I feel like if we go down this route, we'd need specific tests for mobile in iD.detect(), which maybe we should have anyway.

[kepta]

@bhousel , how do we bypass the constructor argument?

[bhousel]

@kepta I think we'd just not use the constructor argument.

Old:

var tags = { amenity: 'parking' };

New:

var tags = new Map().set('amenity', 'parking');

[tmcw]

Re

Is there an advantage to using native Map over Object.create(null)? I think we should do one or the other.

I wrote a post about this, tl;dr is:

• supports non-string keys
• testing for key existence is more foolproof
• easy to test if it's empty
• the objects created by Object.create(null) can be unexpectedly slim - they don't have hasOwnProperty etc, so they can be kind of tricky to work with.

[slhh]

Another idea to address the issue using a normal object:

• define a prefix character, e.g. '*'
• add the prefix character in front of any key starting with the prefix character
• add the prefix character in front of any key which equals a reserved word

We can limit required changes of iD's code to the code parts handling display, user input, and external input/output of the key string. Theoretically, we would also have to add the prefix to specific string constants, but I think we can safely assume that there aren't any string constants for keys starting with '*' or reserved word keys. All other tag handling code can stay unchanged.

As a little drawback, the maximum character number limit for keys starting with '*' would be reduced by 1, but I don't think this would be a significant issue.

[bhousel]

Another idea to address the issue using a normal object:

• define a prefix character, e.g. '*'

Yes, this is the approach that d3.map takes internally, see d3-collection.

Per chat with @kepta the big issue we are running into is that serialization/deserialization with native Map doesn't really work. We need this to be able to save and restore the user's edit history as JSON in localStorage. As @tmcw mentioned in his blog, you can write a function to turn them into Objects, but then they just have all the drawbacks of Objects.

However another thing @kepta found - rauschma wrote about this too: http://2ality.com/2015/08/es6-map-json.html

I like the idea of writing utility functions to convert to/from array pairs. This would work because data in OSM is all strings. It would give us the benefits of using native Maps and still let us serialize them to JSON safely.

[kepta]

I like the idea of writing a utility function which converts Map to Array Pairs and then run JSON.stringify. And using the reverse for parsing. A couple of problems I can see. - handling safe conversion of Map to XML for OSM - when an old iD 's localStorage cache is read by the newer version. Do we have any mechanism for handling breaking caching changes ?

…

On Tue, 11 Apr 2017 at 9:29 PM, Bryan Housel ***@***.***> wrote: Another idea to address the issue using a normal object: - define a prefix character, e.g. '*' Yes, this is the approach that d3.map takes internally, see d3-collection <https://github.com/d3/d3-collection>. Per chat with @kepta <https://github.com/kepta> the big issue we are running into is that serialization/deserialization with native Map doesn't really work. We need this to be able to save and restore the user's edit history as JSON in localStorage. As @tmcw <https://github.com/tmcw> mentioned in his blog, you can write a function to turn them into Objects, but then they just have all the drawbacks of Objects. However another thing @kepta <https://github.com/kepta> found - rauschma wrote about this too: http://2ality.com/2015/08/es6-map-json.html I like the idea of writing utility functions to convert to/from array pairs. This would work because data in OSM is all strings. It would give us the benefits of using native Maps and still let us serialize them to JSON safely. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3044 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGpL7t4m2b5pFjjbl5BJkFBm6ecWNihFks5ru6NYgaJpZM4H4KBE> .

[bhousel]

Also worth mentioning here that I was hoping there might be a lodash oneliner that could convert between Maps and array pairs, but they are not planning support for Maps per lodash/lodash#1121 and lodash/lodash#737, preferring people instead use ES6 features as intended.

This is reasonable but, unfortunately for us, stuff like the spread operator and iterables and for of are not supported on IE11 (and I think we should continue to support it). So to use native Maps and serialize/deserialize them, we will still need to write our own utility functions.

[bhousel]

when an old iD 's localStorage cache is read by the newer version. Do we
have any mechanism for handling breaking caching changes ?

Yes, good thought... They are versioned, thankfully, see here. We would need to make a history version 4.

[kepta]

@bhousel Map.forEach is supported by ie11. Also some of lodash functions like clone, isEqual etc. support Map.

…

On Tue, 11 Apr 2017 at 10:00 PM, Bryan Housel ***@***.***> wrote: Also worth mentioning here that I was hoping there might be a lodash oneliner that could convert between Maps and array pairs, but they are not planning support for Maps per lodash/lodash#1121 <lodash/lodash#1121> and lodash/lodash#737 <lodash/lodash#737>, preferring people instead use ES6 features as intended. This is reasonable but, unfortunately for us, stuff like the spread operator and iterables and for of are not supported on IE11 (and I think we should continue to support it). So to use native Maps and serialize/deserialize them, we will still need to write our own utility functions. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3044 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGpL7pkmovQ4xLZJNUNoo_m8onJY9LTnks5ru6qkgaJpZM4H4KBE> .