Should Map be moved to Common Code

[aadish]

GEE uses Map.js which is copy pasted from Sugar and Salt Solutions. Should this be moved to common code?

Tagging it for developer meeting

[samreid]

How well would it work to rewrite the code using JS attributes to avoid the need for a separate Map instance?

For instance, instead of

          // Enter this connection in the map.
          this.mapRibosomeToShapeSegment.put( ribosome, this.shapeSegments[ 0 ] );

it would read something like:

          // Enter this connection in the map.
          ribosome.shapeSegment = this.shapeSegments[0];

and we would eliminate the map completely (may need to track ribosomes in an array though)?

[pixelzoom]

If the code can't be rewritten and Map is still used, then yes - Map should be generalized (if it's not already general) and migrated to common code.

[Denz1994]

Is Map something that would go in phetsims/DOT?

[samreid]

We should try to implement this without a separate map structure as described in https://github.com/phetsims/gene-expression-essentials/issues/63#issuecomment-295571675. If you need to use a map data structure implementation, we should investigate https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map before adding our own to common code.

[jonathanolson]

Is Map something that would go in phetsims/DOT?

If it's moved to common code, PHET_CORE sounds better.

For this type of behavior, I've added IDs or another unique identifier to key objects so that lookup time is fast (instead of doing a full scan).

Map does make this more convenient (for when performance isn't required as fast as possible). If generalizing, using splice() instead of delete may be preferred, depending on how browsers handle sparse arrays (delete creates gaps and indices will tend to increase).

[jbphet]

Discussed at 4/20/2017 developer meeting, here are the recommendations in order of preference:

• Investigate using the relatively new JavaScript standard version that @samreid mentions above. We will need to determine if it is available on all of our currently supported platforms.
• Investigate whether a common library, particularly one that we already use (such as lodash/underscore), has something that we can easily use instead
• Rewrite the code to not need this if it is reasonably easy to do so
• Move the code into common code

Assigning back to @aadish to follow through on the steps listed above.

[jbphet]

Once a solution is decided upon, a TODO item should be added in the code for Sugar and Salt Solutions that says something like:

  // TODO: If and when development resumes on this simulation, this Map function should be replaced
  // with <insert brief description of solution here>, see <link to this issue> for more information.

[aadish]

referring commits fca3683960f4da570e55162bb07663bef1e72f8c, https://github.com/phetsims/chipper/commit/8cd7f43b40b86c789f7e67f56fd0b8a52510577c,

[aadish]

Going ahead with the new JavaScript standard suggested above, tested across platforms and it is supported

Tested on Chrome: 57.0.2987.133 Edge: 40.15063.0.0 IE 11 Safari: 8 and 9 on IPad Firefox: 53.0

Keeping this issue open to verify during RC

[pixelzoom]

In 6/12/17 Skype discussion, we concluded that PhET code should not be using JS Map.

[pixelzoom]

More 6/12/17 Skype discussion... Wrapping JS Map so that (a) only supported features are usable and (b) missing features can be provided seems like a fine alternative. But don't use JS Map directly.

[pixelzoom]

Still more 6/12/17 Skype discussion... JS Map is not supported on Android mobile. That seems significant with the forthcoming PhET Android app.

[samreid]

That seems significant with the forthcoming PhET Android app.

@mattpen can you comment on what technology the android webview uses?

[mattpen]

From https://developer.chrome.com/multidevice/webview/overview

Since Android 4.4 (KitKat), the WebView component is based on the Chromium open source project. WebViews now include an updated version of the V8 JavaScript engine

The WebView shipped with Android 4.4 (KitKat) is based on the same code as Chrome for Android version 30. This WebView does not have full feature parity with Chrome for Android and is given the version number 30.0.0.0.

It looks like Chrome for Android didn't support map until 38: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map#AutoCompatibilityTable. We are currently still planning to support KitKat which means we will not be able to use Map, although that is a flexible requirement. In the next major release of Android (Lollipop 5.0), WebView is allowed to upgrade without upgrading the OS, so if use of Map is critical we can consider only providing the app on Lollipop and higher.

[samreid]

use of Map is critical we can consider only providing the app on Lollipop and higher.

Use of Map is not critical, we should blacklist it for a year then re-evaluate.

[jbphet]

Assigning to myself to rewrite the portions that use the Map construct.

[jbphet]

I've rewritten the code so that the JavaScript Map construct is no longer needed nor used. Closing.