Skip to content

Commit

Permalink
Fix GraphQL error when switching between pipelines with different modes
Browse files Browse the repository at this point in the history
Summary: The error toasts that appeared when you switched pipelines in the execute tab were caused by the local storage provider, which was vending data for the wrong pipeline for one render cycle because it was keeping it's own state in sync with a useEffect (componentDidUpdate) hook.

Test Plan: Run and see no errors!

Reviewers: #ft, alangenfeld

Reviewed By: #ft, alangenfeld

Subscribers: alangenfeld

Differential Revision: https://dagster.phacility.com/D1099
  • Loading branch information
bengotow committed Sep 27, 2019
1 parent 2462e85 commit ae8277f
Showing 1 changed file with 28 additions and 12 deletions.
40 changes: 28 additions & 12 deletions js_modules/dagit/src/LocalStorage.tsx
Expand Up @@ -101,7 +101,14 @@ export type StorageHook = [
React.Dispatch<React.SetStateAction<IStorageData>>
];

let _data: IStorageData | null = null;
let _dataNamespace = "";

function getStorageDataForNamespace(namespace: string) {
if (_data && _dataNamespace === namespace) {
return _data;
}

let data: IStorageData = {
sessions: {},
current: ""
Expand All @@ -120,24 +127,33 @@ function getStorageDataForNamespace(namespace: string) {
if (!data.sessions[data.current]) {
data.current = Object.keys(data.sessions)[0];
}

_data = data;
_dataNamespace = namespace;

return data;
}

export function useStorage({ namespace }: { namespace: string }): StorageHook {
const [data, setData] = React.useState<IStorageData>(() =>
getStorageDataForNamespace(namespace)
);
function writeStorageDataForNamespace(namespace: string, data: IStorageData) {
_data = data;
_dataNamespace = namespace;
window.localStorage.setItem(`dagit.${namespace}`, JSON.stringify(data));
}

React.useEffect(() => {
// When the namespace changes, re-initialize the data by reading from LocalStorage
// and running a few consistency checks to ensure a valid state.
setData(getStorageDataForNamespace(namespace));
}, [namespace]);
/* React hook that provides local storage to the caller. A previous version of this
loaded data into React state, but changing namespaces caused the data to be out-of-sync
for one render (until a useEffect could update the data in state). Now we keep the
current localStorage namespace in memory (in _data above) and React keeps a simple
version flag it can use to trigger a re-render after changes are saved, so changing
namespaces changes the returned data immediately.
*/
export function useStorage({ namespace }: { namespace: string }): StorageHook {
const [version, setVersion] = React.useState<number>(0);

const onSave = (newData: IStorageData) => {
setData(newData);
window.localStorage.setItem(`dagit.${namespace}`, JSON.stringify(newData));
writeStorageDataForNamespace(namespace, newData);
setVersion(version + 1); // trigger a React render
};

return [data, onSave];
return [getStorageDataForNamespace(namespace), onSave];
}

0 comments on commit ae8277f

Please sign in to comment.