Ensure we don't keep "spurious" changes on implementation updates

Also this ensures weird stuff doesn't happen when we change arg/global names.

lib/client-api/src/new/ArgsStore.test.ts

@@ -2,6 +2,8 @@ import { ArgsStore } from './ArgsStore';
 
 jest.mock('@storybook/client-logger');
 
+const stringType = { type: { name: 'string' } };
+
 describe('ArgsStore', () => {
   describe('setInitial / get', () => {
     it('returns in a straightforward way', () => {
@@ -57,7 +59,7 @@ describe('ArgsStore', () => {
 
       const story = {
         id: 'id',
-        argTypes: { a: { type: { name: 'string' } } },
+        argTypes: { a: stringType },
       } as any;
       store.updateFromPersisted(story, { a: 'str' });
       expect(store.get('id')).toEqual({ a: 'str' });
@@ -120,12 +122,14 @@ describe('ArgsStore', () => {
         const previousStory = {
           id: 'id',
           initialArgs: { a: '1', b: '1' },
+          argTypes: { a: stringType, b: stringType },
         } as any;
         store.setInitial('id', previousStory.initialArgs);
 
         const story = {
           id: 'id',
           initialArgs: { a: '1', b: '1' },
+          argTypes: { a: stringType, b: stringType },
         } as any;
 
         store.resetOnImplementationChange(story, previousStory);
@@ -138,18 +142,22 @@ describe('ArgsStore', () => {
         const previousStory = {
           id: 'id',
           initialArgs: { a: '1', b: '1' },
+          argTypes: { a: stringType, b: stringType },
         } as any;
         store.setInitial('id', previousStory.initialArgs);
 
+        // NOTE: I'm not sure technically you should be allowed to set c here
         store.update('id', { a: 'update', c: 'update' });
 
         const story = {
           id: 'id',
           initialArgs: { a: '1', b: '1' },
+          argTypes: { a: stringType, b: stringType },
         } as any;
 
         store.resetOnImplementationChange(story, previousStory);
-        expect(store.get(story.id)).toEqual({ a: 'update', b: '1', c: 'update' });
+        // In any case c is not retained.
+        expect(store.get(story.id)).toEqual({ a: 'update', b: '1' });
       });
     });
 
@@ -160,12 +168,14 @@ describe('ArgsStore', () => {
         const previousStory = {
           id: 'id',
           initialArgs: { a: '1', b: '1' },
+          argTypes: { a: stringType, b: stringType },
         } as any;
         store.setInitial('id', previousStory.initialArgs);
 
         const story = {
           id: 'id',
           initialArgs: { a: '1', c: '1' },
+          argTypes: { a: stringType, c: stringType },
         } as any;
 
         store.resetOnImplementationChange(story, previousStory);
@@ -178,18 +188,22 @@ describe('ArgsStore', () => {
         const previousStory = {
           id: 'id',
           initialArgs: { a: '1', b: '1' },
+          argTypes: { a: stringType, b: stringType },
         } as any;
         store.setInitial('id', previousStory.initialArgs);
 
+        // NOTE: I'm not sure technically you should be allowed to set c here
         store.update('id', { a: 'update', c: 'update' });
 
         const story = {
           id: 'id',
           initialArgs: { a: '2', d: '2' },
+          argTypes: { a: stringType, d: stringType },
         } as any;
 
         store.resetOnImplementationChange(story, previousStory);
-        expect(store.get(story.id)).toEqual({ a: 'update', c: 'update', d: '2' });
+        // In any case c is not retained.
+        expect(store.get(story.id)).toEqual({ a: 'update', d: '2' });
       });
     });
   });

lib/client-api/src/new/ArgsStore.ts

@@ -35,10 +35,10 @@ export class ArgsStore {
   resetOnImplementationChange(story: Story<any>, previousStory: Story<any>) {
     const delta = deepDiff(previousStory.initialArgs, this.get(story.id));
 
-    const newArgs =
-      delta === DEEPLY_EQUAL ? story.initialArgs : combineArgs(story.initialArgs, delta);
-
-    this.argsByStoryId[story.id] = newArgs;
+    this.argsByStoryId[story.id] = story.initialArgs;
+    if (delta !== DEEPLY_EQUAL) {
+      this.updateFromPersisted(story, delta);
+    }
   }
 
   update(storyId: StoryId, argsUpdate: Partial<Args>) {

lib/client-api/src/new/GlobalsStore.test.ts

@@ -40,13 +40,17 @@ describe('GlobalsStore', () => {
 
   describe('update', () => {
     it('changes the global args', () => {
-      const store = new GlobalsStore({ globals: {}, globalTypes: {} });
+      const store = new GlobalsStore({ globals: { foo: 'old' }, globalTypes: { baz: {} } });
 
       store.update({ foo: 'bar' });
       expect(store.get()).toEqual({ foo: 'bar' });
 
       store.update({ baz: 'bing' });
       expect(store.get()).toEqual({ foo: 'bar', baz: 'bing' });
+
+      // NOTE: this is currently allowed but deprecated.
+      store.update({ random: 'value' });
+      expect(store.get()).toEqual({ foo: 'bar', baz: 'bing', random: 'value' });
     });
 
     it('does not merge objects', () => {
@@ -107,7 +111,7 @@ describe('GlobalsStore', () => {
     });
 
     describe('when underlying globals have not changed', () => {
-      it('retains updated values', () => {
+      it('retains updated values, but not if they are undeclared', () => {
         const store = new GlobalsStore({
           globals: {
             arg1: 'arg1',
@@ -123,6 +127,7 @@ describe('GlobalsStore', () => {
           arg3: 'new-arg3',
         });
 
+        // You can set undeclared values (currently, deprecated)
         expect(store.get()).toEqual({ arg1: 'new-arg1', arg2: 'new-arg2', arg3: 'new-arg3' });
 
         store.resetOnGlobalMetaChange({
@@ -133,7 +138,8 @@ describe('GlobalsStore', () => {
             arg2: { defaultValue: 'arg2' },
           },
         });
-        expect(store.get()).toEqual({ arg1: 'new-arg1', arg2: 'new-arg2', arg3: 'new-arg3' });
+        // However undeclared valuse aren't persisted
+        expect(store.get()).toEqual({ arg1: 'new-arg1', arg2: 'new-arg2' });
       });
     });
 
@@ -158,6 +164,7 @@ describe('GlobalsStore', () => {
         expect(store.get()).toEqual({
           arg1: 'new-arg1',
           arg2: 'new-arg2',
+          // You can set undeclared values (currently, deprecated)
           arg3: 'new-arg3',
           arg4: 'arg4',
         });
@@ -171,10 +178,9 @@ describe('GlobalsStore', () => {
             arg5: { defaultValue: 'edited-arg5' },
           },
         });
+        // However undeclared values aren't persisted
         expect(store.get()).toEqual({
           arg1: 'new-arg1',
-          arg2: 'new-arg2',
-          arg3: 'new-arg3',
           arg4: 'edited-arg4',
           arg5: 'edited-arg5',
         });

lib/client-api/src/new/GlobalsStore.ts

@@ -1,6 +1,17 @@
+import deprecate from 'util-deprecate';
+import dedent from 'ts-dedent';
+
 import { Globals, GlobalTypes } from './types';
 import { combineArgs, deepDiff, DEEPLY_EQUAL } from '../args';
 
+const setUndeclaredWarning = deprecate(
+  () => {},
+  dedent`
+    Setting a global value that is undeclared (i.e. not in the user's initial set of globals
+    or globalTypes) is deprecated and will have no effect in 7.0.
+  `
+);
+
 export class GlobalsStore {
   allowedGlobalNames: Set<string>;
 
@@ -18,19 +29,22 @@ export class GlobalsStore {
   setInitialGlobals({ globals, globalTypes }: { globals: Globals; globalTypes: GlobalTypes }) {
     this.allowedGlobalNames = new Set([...Object.keys(globals), ...Object.keys(globalTypes)]);
 
-    const defaultGlobals = Object.entries(globalTypes).reduce((acc, [arg, { defaultValue }]) => {
-      if (defaultValue) acc[arg] = defaultValue;
+    const defaultGlobals = Object.entries(globalTypes).reduce((acc, [key, { defaultValue }]) => {
+      if (defaultValue) acc[key] = defaultValue;
       return acc;
     }, {} as Globals);
     this.initialGlobals = { ...defaultGlobals, ...globals };
   }
 
-  updateFromPersisted(persisted: Globals) {
-    const allowedUrlGlobals = Object.entries(persisted).reduce((acc, [key, value]) => {
+  filterAllowedGlobals(globals: Globals) {
+    return Object.entries(globals).reduce((acc, [key, value]) => {
       if (this.allowedGlobalNames.has(key)) acc[key] = value;
       return acc;
     }, {} as Globals);
+  }
 
+  updateFromPersisted(persisted: Globals) {
+    const allowedUrlGlobals = this.filterAllowedGlobals(persisted);
     // Note that unlike args, we do not have the same type information for globals to allow us
     // to type check them here, so we just set them naively
     this.globals = { ...this.globals, ...allowedUrlGlobals };
@@ -47,15 +61,23 @@ export class GlobalsStore {
 
     this.setInitialGlobals({ globals, globalTypes });
 
-    this.globals =
-      delta === DEEPLY_EQUAL ? this.initialGlobals : combineArgs(this.initialGlobals, delta);
+    this.globals = this.initialGlobals;
+    if (delta !== DEEPLY_EQUAL) {
+      this.updateFromPersisted(delta);
+    }
   }
 
   get() {
     return this.globals;
   }
 
   update(newGlobals: Globals) {
+    Object.keys(newGlobals).forEach((key) => {
+      if (!this.allowedGlobalNames.has(key)) {
+        setUndeclaredWarning();
+      }
+    });
+
     this.globals = { ...this.globals, ...newGlobals };
   }
 }