Merge pull request #1124 from plotly/cast-frame-names-to-string

Enforce casting requested frame names to strings

Author: rreusser

src/plot_api/plot_api.js

@@ -2258,7 +2258,6 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) {
                     frameOpts: frameOpts,
                     transitionOpts: transitionOpts,
                 };
-
                 if(i === frameList.length - 1) {
                     // The last frame in this .animate call stores the promise resolve
                     // and reject callbacks. This is how we ensure that the animation
@@ -2410,14 +2409,15 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) {
         } else if(isFrameArray) {
             for(i = 0; i < frameOrGroupNameOrFrameList.length; i++) {
                 var frameOrName = frameOrGroupNameOrFrameList[i];
-                if(typeof frameOrName === 'string') {
+                if(['number', 'string'].indexOf(typeof frameOrName) !== -1) {
+                    frameOrName = String(frameOrName);
                     // In this case, there's an array and this frame is a string name:
                     frameList.push({
                         type: 'byname',
                         name: frameOrName,
                         data: setTransitionConfig({name: frameOrName})
                     });
-                } else {
+                } else if(Lib.isPlainObject(frameOrName)) {
                     frameList.push({
                         type: 'object',
                         data: setTransitionConfig(Lib.extendFlat({}, frameOrName))

src/plots/plots.js

@@ -1798,6 +1798,9 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts)
     }
 
     function completeTransition(callback) {
+        // Fail-safe against purged plot:
+        if(!gd._transitionData) return;
+
         flushCallbacks(gd._transitionData._interruptCallbacks);
 
         return Promise.resolve().then(function() {
@@ -1815,6 +1818,9 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts)
     }
 
     function interruptPreviousTransitions() {
+        // Fail-safe against purged plot:
+        if(!gd._transitionData) return;
+
         // If a transition is interrupted, set this to false. At the moment, the only thing that would
         // interrupt a transition is another transition, so that it will momentarily be set to true
         // again, but this determines whether autorange or dragbox work, so it's for the sake of

test/jasmine/tests/animate_test.js

@@ -88,6 +88,14 @@ describe('Test animate API', function() {
 
         mockCopy = Lib.extendDeep({}, mock);
 
+        // ------------------------------------------------------------
+        // NB: TRANSITION IS FAKED
+        //
+        // This means that you should not expect `.animate` to actually
+        // modify the plot in any way in the tests below. For tests
+        // involvingnon-faked transitions, see the bottom of this file.
+        // ------------------------------------------------------------
+
         spyOn(Plots, 'transition').and.callFake(function() {
             // Transition's fake behavior is just to delay by the duration
             // and resolve:
@@ -579,3 +587,48 @@ describe('Test animate API', function() {
         });
     });
 });
+
+describe('Test animate API', function() {
+    'use strict';
+
+    var gd, mockCopy;
+
+    beforeEach(function(done) {
+        gd = createGraphDiv();
+        mockCopy = Lib.extendDeep({}, mock);
+        Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(function() {
+            return Plotly.addFrames(gd, mockCopy.frames);
+        }).then(done);
+    });
+
+    afterEach(function() {
+        Plotly.purge(gd);
+        destroyGraphDiv();
+    });
+
+    it('does not fail if strings are not used', function(done) {
+        Plotly.addFrames(gd, [{name: 8, data: [{x: [8, 7, 6]}]}]).then(function() {
+            // Verify it was added as a string name:
+            expect(gd._transitionData._frameHash['8']).not.toBeUndefined();
+
+            // Transition using a number:
+            return Plotly.animate(gd, [8], {transition: {duration: 0}, frame: {duration: 0}});
+        }).then(function() {
+            // Confirm the result:
+            expect(gd.data[0].x).toEqual([8, 7, 6]);
+        }).catch(fail).then(done);
+    });
+
+    it('ignores null and undefined frames', function(done) {
+        var cnt = 0;
+        gd.on('plotly_animatingframe', function() {cnt++;});
+
+        Plotly.animate(gd, ['frame0', null, undefined], {transition: {duration: 0}, frame: {duration: 0}}).then(function() {
+            // Check only one animating was fired:
+            expect(cnt).toEqual(1);
+
+            // Check unused frames did not affect the current frame:
+            expect(gd._fullLayout._currentFrame).toEqual('frame0');
+        }).catch(fail).then(done);
+    });
+});

test/jasmine/tests/frame_api_test.js

@@ -60,6 +60,12 @@ describe('Test frame api', function() {
             }).catch(fail).then(done);
         });
 
+        it('casts names to strings', function(done) {
+            Plotly.addFrames(gd, [{name: 5}]).then(function() {
+                expect(Object.keys(h)).toEqual(['5']);
+            }).catch(fail).then(done);
+        });
+
         it('creates multiple unnamed frames at the same time', function(done) {
             Plotly.addFrames(gd, [{}, {}]).then(function() {
                 expect(f).toEqual([{name: 'frame 0'}, {name: 'frame 1'}]);