From 6caae9eccc8acfad378e5c75628016973b4902cd Mon Sep 17 00:00:00 2001 From: Marin Minnerly Date: Fri, 20 Dec 2024 12:44:29 -0800 Subject: [PATCH] Use a new function pair for saving and loading paths --- src/Common/getInstanceFromFullName.luau | 64 ------------------- src/Common/getInstanceFromPath.luau | 44 +++++++++++++ ...pec.luau => getInstanceFromPath.spec.luau} | 25 ++++---- src/Common/getInstancePath.luau | 18 ++++++ src/Common/getInstancePath.spec.luau | 48 ++++++++++++++ src/Plugin/LocalStorageContext.luau | 34 +++++----- src/Storybook/useLastOpenedStory.luau | 13 ++-- src/TreeView/usePinnedInstances.luau | 11 ++-- 8 files changed, 150 insertions(+), 107 deletions(-) delete mode 100644 src/Common/getInstanceFromFullName.luau create mode 100644 src/Common/getInstanceFromPath.luau rename src/Common/{getInstanceFromFullName.spec.luau => getInstanceFromPath.spec.luau} (62%) create mode 100644 src/Common/getInstancePath.luau create mode 100644 src/Common/getInstancePath.spec.luau diff --git a/src/Common/getInstanceFromFullName.luau b/src/Common/getInstanceFromFullName.luau deleted file mode 100644 index d11dd606..00000000 --- a/src/Common/getInstanceFromFullName.luau +++ /dev/null @@ -1,64 +0,0 @@ ---[[ - Gets an instance based off the result of GetFullName(). - - This is used in conjunction with debug.info() to locate the calling script. - - Returns nil if the instance is outside the DataModel. -]] - -local Sift = require("@pkg/Sift") - -local tryGetService = require("@root/RobloxInternal/tryGetService") - -local PATH_SEPERATOR = "." - -local function getInstanceFromFullName(fullName: string): Instance? - local parts = fullName:split(PATH_SEPERATOR) - local serviceName = table.remove(parts, 1) - - if serviceName then - -- This function only works for instances in the DataModel. As such, the - -- first part of the path will always be a service, so if we can't find - -- one we exit out and return nil - local current = tryGetService(serviceName) - - if current then - while #parts > 0 do - -- Keep around a copy of the `parts` array. We are going to concat this - -- into new paths, and incrementally remove from the right to narrow - -- down the file path. - local tempParts = Sift.Array.copy(parts) - - -- The result of GetFullName() uses dots to separate paths, but we also - -- use dots in our file names (e.g. with spec and story files). As such, - -- this block will look ahead to see if multiple parts are actually a - -- single filename. - for _ = 1, #tempParts do - local name = table.concat(tempParts, PATH_SEPERATOR) - local found = current:FindFirstChild(name) - - if found then - current = found - parts = Sift.List.shift(parts, #name:split(PATH_SEPERATOR)) - break - else - if #tempParts == 1 then - -- This fixes a crash when searching for paths that - -- no longer exist - return nil - else - -- Reduce from the right until we find the next instance - tempParts = Sift.List.pop(tempParts) - end - end - end - end - - return current - end - end - - return nil -end - -return getInstanceFromFullName diff --git a/src/Common/getInstanceFromPath.luau b/src/Common/getInstanceFromPath.luau new file mode 100644 index 00000000..a28c0d7d --- /dev/null +++ b/src/Common/getInstanceFromPath.luau @@ -0,0 +1,44 @@ +--[[ + Gets an instance based off the result of getInstancePath. + + The reason we use this over GetFullName is because it uses a dot (.) as the + path separator which makes it difficult to disambiguate instances stories + and test files (Foo.story and Foo.spec, respectively) + + Returns `nil` if the instance is outside the DataModel or otherwise cannot + be found. +]] + +local tryGetService = require("@root/RobloxInternal/tryGetService") + +local PATH_SEPERATOR = "/" + +local function getInstanceFromPath(path: string): Instance? + local parts = path:split(PATH_SEPERATOR) + local serviceName = parts[1] + + if serviceName then + -- This function only works for instances in the DataModel. As such, the + -- first part of the path will always be a service, so if we can't find + -- one we exit out and return nil + local current = tryGetService(serviceName) + + if current then + for i = 2, #parts do + local found = current:FindFirstChild(parts[i]) + + if found then + current = found + else + return nil + end + end + + return current + end + end + + return nil +end + +return getInstanceFromPath diff --git a/src/Common/getInstanceFromFullName.spec.luau b/src/Common/getInstanceFromPath.spec.luau similarity index 62% rename from src/Common/getInstanceFromFullName.spec.luau rename to src/Common/getInstanceFromPath.spec.luau index d110cbda..a343e4a4 100644 --- a/src/Common/getInstanceFromFullName.spec.luau +++ b/src/Common/getInstanceFromPath.spec.luau @@ -3,7 +3,8 @@ local ReplicatedStorage = game:GetService("ReplicatedStorage") local JestGlobals = require("@pkg/JestGlobals") local newFolder = require("@root/Testing/newFolder") -local getInstanceFromFullName = require("./getInstanceFromFullName") +local getInstanceFromPath = require("./getInstanceFromPath") +local getInstancePath = require("./getInstancePath") local expect = JestGlobals.expect local test = JestGlobals.test @@ -18,7 +19,7 @@ afterEach(function() end) test("gets services", function() - local path = getInstanceFromFullName("ReplicatedStorage") + local path = getInstanceFromPath("ReplicatedStorage") expect(path).toBe(ReplicatedStorage) end) @@ -32,8 +33,7 @@ test("works on nested instances", function() }) folder.Parent = ReplicatedStorage - local path = getInstanceFromFullName(module:GetFullName()) - expect(path).toBe(module) + expect(getInstanceFromPath(getInstancePath(module))).toBe(module) end) test("works with spec files", function() @@ -46,7 +46,7 @@ test("works with spec files", function() }) folder.Parent = ReplicatedStorage - local path = getInstanceFromFullName(module:GetFullName()) + local path = getInstanceFromPath(module:GetFullName()) expect(path).toBe(module) end) @@ -61,18 +61,17 @@ test("finds spec files BEFORE the module it is associated with", function() }) folder.Parent = ReplicatedStorage - local path = getInstanceFromFullName(module:GetFullName()) - expect(path).toBe(module) + expect(getInstanceFromPath(getInstancePath(module))).toBe(module) end) test("returns nil if the first part of the path is not a service", function() - expect(getInstanceFromFullName("Part")).toBeUndefined() + expect(getInstanceFromPath("Part")).toBeUndefined() end) test("returns nil if the path does not exist", function() - expect(getInstanceFromFullName("Foo.story")).toBeUndefined() - expect(getInstanceFromFullName("Path.To.Foo.story")).toBeUndefined() - expect(getInstanceFromFullName("ReplicatedStorage.Foo.Bar.Baz")).toBeUndefined() - expect(getInstanceFromFullName("ReplicatedStorage.Sample.story")).toBeUndefined() - expect(getInstanceFromFullName("ReplicatedStorage.Sample.spec")).toBeUndefined() + expect(getInstanceFromPath("Foo.story")).toBeUndefined() + expect(getInstanceFromPath("Path/To/Foo.story")).toBeUndefined() + expect(getInstanceFromPath("ReplicatedStorage/Foo/Bar/Baz")).toBeUndefined() + expect(getInstanceFromPath("ReplicatedStorage/Sample.story")).toBeUndefined() + expect(getInstanceFromPath("ReplicatedStorage/Sample.spec")).toBeUndefined() end) diff --git a/src/Common/getInstancePath.luau b/src/Common/getInstancePath.luau new file mode 100644 index 00000000..082df3e6 --- /dev/null +++ b/src/Common/getInstancePath.luau @@ -0,0 +1,18 @@ +local PATH_SEPARATOR = "/" + +local function getInstancePath(instance: Instance, pathSeparator: string?): string + pathSeparator = if pathSeparator then pathSeparator else PATH_SEPARATOR + assert(pathSeparator, "Luau") + + local path = {} + local current = instance + + while current and current.Parent ~= nil do + table.insert(path, 1, current.Name) + current = current.Parent + end + + return table.concat(path, "/") +end + +return getInstancePath diff --git a/src/Common/getInstancePath.spec.luau b/src/Common/getInstancePath.spec.luau new file mode 100644 index 00000000..1ee401ba --- /dev/null +++ b/src/Common/getInstancePath.spec.luau @@ -0,0 +1,48 @@ +local ReplicatedStorage = game:GetService("ReplicatedStorage") + +local JestGlobals = require("@pkg/JestGlobals") +local newFolder = require("@root/Testing/newFolder") + +local getInstancePath = require("./getInstancePath") + +local expect = JestGlobals.expect +local test = JestGlobals.test +local afterEach = JestGlobals.afterEach + +local folder: Folder + +afterEach(function() + if folder then + folder:Destroy() + end +end) + +test("services are treated as the root", function() + expect(getInstancePath(ReplicatedStorage)).toBe("ReplicatedStorage") +end) + +test("works on nested instances", function() + local module = Instance.new("ModuleScript") + + folder = newFolder({ + foo = newFolder({ + bar = module, + }), + }) + folder.Parent = ReplicatedStorage + + expect(getInstancePath(module)).toBe("ReplicatedStorage/Folder/foo/bar") +end) + +test("works with spec files", function() + local module = Instance.new("ModuleScript") + + folder = newFolder({ + foo = newFolder({ + ["bar.spec"] = module, + }), + }) + folder.Parent = ReplicatedStorage + + expect(getInstancePath(module)).toBe("ReplicatedStorage/Folder/foo/bar.spec") +end) diff --git a/src/Plugin/LocalStorageContext.luau b/src/Plugin/LocalStorageContext.luau index bd049b0d..0f705cb6 100644 --- a/src/Plugin/LocalStorageContext.luau +++ b/src/Plugin/LocalStorageContext.luau @@ -4,12 +4,9 @@ local React = require("@pkg/React") local Sift = require("@pkg/Sift") local PluginContext = require("@root/Plugin/PluginContext") -local usePrevious = require("@root/Common/usePrevious") local useCallback = React.useCallback local useContext = React.useContext -local useEffect = React.useEffect -local useRef = React.useRef local useState = React.useState export type LocalStorage = { @@ -42,15 +39,16 @@ local function LocalStorageProvider(props: Props) return {} end, { plugin, props.storageKey } :: { unknown }) - local storage, setStorage = useState(loadFromDisk) - local prevStorage = usePrevious(storage) - - local saveToDisk = useCallback(function() - local data = HttpService:JSONEncode(storage) + local saveToDisk = useCallback(function(newStorage: { [any]: any }) + local data = HttpService:JSONEncode(newStorage) if data then plugin:SetSetting(props.storageKey, data) end - end, { plugin, props.storageKey, storage } :: { unknown }) + end, { plugin, props.storageKey } :: { unknown }) + + local storage, setStorage = useState(function() + return loadFromDisk() + end) local get = useCallback(function(key: string) return storage[key] @@ -58,25 +56,23 @@ local function LocalStorageProvider(props: Props) local set = useCallback(function(key: string, value: unknown?) setStorage(function(prev) - return Sift.Dictionary.join(prev, { + local new = Sift.Dictionary.join(prev, { [key] = if value == nil then Sift.None else value, }) + + saveToDisk(new) + + return new end) end, { storage }) - useEffect(function() - if storage and storage ~= prevStorage then - saveToDisk() - end - end, { storage, prevStorage, saveToDisk } :: { unknown }) - - local context: LocalStorageContext = useRef({ + local context: LocalStorageContext = { get = get, set = set, - }) + } return React.createElement(LocalStorageContext.Provider, { - value = context.current, + value = context, }, props.children) end diff --git a/src/Storybook/useLastOpenedStory.luau b/src/Storybook/useLastOpenedStory.luau index b59457f5..8165ca0f 100644 --- a/src/Storybook/useLastOpenedStory.luau +++ b/src/Storybook/useLastOpenedStory.luau @@ -2,7 +2,8 @@ local React = require("@pkg/React") local LocalStorageContext = require("@root/Plugin/LocalStorageContext") local SettingsContext = require("@root/UserSettings/SettingsContext") -local getInstanceFromFullName = require("@root/Common/getInstanceFromFullName") +local getInstanceFromPath = require("@root/Common/getInstanceFromPath") +local getInstancePath = require("@root/Common/getInstancePath") local useCallback = React.useCallback local useMemo = React.useMemo @@ -15,8 +16,8 @@ local function useLastOpenedStory(): (ModuleScript?, (storyModule: ModuleScript? local settingsContext = SettingsContext.use() local setLastOpenedStory = useCallback(function(storyModule: ModuleScript?) - localStorage.set(LAST_OPENED_STORY_PATH_KEY, if storyModule then storyModule:GetFullName() else nil) - end, { localStorage }) + localStorage.set(LAST_OPENED_STORY_PATH_KEY, if storyModule then getInstancePath(storyModule) else nil) + end, { localStorage.set }) local lastOpenedStory = useMemo(function(): ModuleScript? local rememberLastOpenedStory = settingsContext.getSetting(REMEMBER_LAST_OPENED_STORY_KEY) @@ -27,8 +28,8 @@ local function useLastOpenedStory(): (ModuleScript?, (storyModule: ModuleScript? local lastOpenedStoryPath = localStorage.get(LAST_OPENED_STORY_PATH_KEY) - if lastOpenedStoryPath then - local instance = getInstanceFromFullName(lastOpenedStoryPath) + if lastOpenedStoryPath and typeof(lastOpenedStoryPath) == "string" then + local instance = getInstanceFromPath(lastOpenedStoryPath) if instance and instance:IsA("ModuleScript") then return instance @@ -36,7 +37,7 @@ local function useLastOpenedStory(): (ModuleScript?, (storyModule: ModuleScript? end return nil - end, { settingsContext, localStorage }) + end, { settingsContext, localStorage.get }) return lastOpenedStory, setLastOpenedStory end diff --git a/src/TreeView/usePinnedInstances.luau b/src/TreeView/usePinnedInstances.luau index 22b8a74b..6644329d 100644 --- a/src/TreeView/usePinnedInstances.luau +++ b/src/TreeView/usePinnedInstances.luau @@ -2,7 +2,8 @@ local React = require("@pkg/React") local Sift = require("@pkg/Sift") local LocalStorageContext = require("@root/Plugin/LocalStorageContext") -local getInstanceFromFullName = require("@root/Common/getInstanceFromFullName") +local getInstanceFromPath = require("@root/Common/getInstanceFromPath") +local getInstancePath = require("@root/Common/getInstancePath") local usePrevious = require("@root/Common/usePrevious") local useCallback = React.useCallback @@ -26,14 +27,14 @@ local function usePinnedInstances(): (ModuleScript?, (storyModule: ModuleScript? local pin = useCallback(function(instance: Instance) setPinnedPaths(function(prev) - return Sift.List.append(prev, instance:GetFullName()) + return Sift.List.append(prev, getInstancePath(instance)) end) end, {}) local unpin = useCallback(function(instance: Instance) setPinnedPaths(function(prev) return Sift.List.filter(prev, function(pinnedPath) - return pinnedPath ~= instance:GetFullName() + return pinnedPath ~= getInstancePath(instance) end) end) end, {}) @@ -44,7 +45,7 @@ local function usePinnedInstances(): (ModuleScript?, (storyModule: ModuleScript? for _, pinnedPath in pinnedPaths do table.insert(pinnedInstances, { path = pinnedPath, - instance = getInstanceFromFullName(pinnedPath), + instance = getInstanceFromPath(pinnedPath), }) end @@ -67,7 +68,7 @@ local function usePinnedInstances(): (ModuleScript?, (storyModule: ModuleScript? if prevPinnedPaths and pinnedPaths ~= prevPinnedPaths then localStorage.set(PINNED_INSTANCES_KEY, pinnedPaths) end - end, { localStorage, pinnedPaths, prevPinnedPaths }) + end, { localStorage.set, pinnedPaths, prevPinnedPaths }) return { pin = pin,