From 02f612a65d735a8fea39abb67c73108341391d92 Mon Sep 17 00:00:00 2001 From: kevin Date: Mon, 23 Dec 2024 11:34:13 -0700 Subject: [PATCH] refactor(toolbox/getcontext): refactoring to not throw, but instead log errors --- .../src/getContext/getContext.test.ts | 119 ++++++++++-------- .../snap-toolbox/src/getContext/getContext.ts | 23 ++-- 2 files changed, 82 insertions(+), 60 deletions(-) diff --git a/packages/snap-toolbox/src/getContext/getContext.test.ts b/packages/snap-toolbox/src/getContext/getContext.test.ts index 0cbf4b818..d9206d99f 100644 --- a/packages/snap-toolbox/src/getContext/getContext.test.ts +++ b/packages/snap-toolbox/src/getContext/getContext.test.ts @@ -301,25 +301,47 @@ describe('getContext', () => { }).not.toThrow(); }); - it('throws an error when JavaScript keywords are provided in the evaluate array', () => { + it('logs an error when there is invalid syntax in the script context', () => { const scriptTag = document.createElement('script'); scriptTag.setAttribute('type', 'searchspring'); + scriptTag.innerHTML = ` + valid = 'valid'; + invalid = syntax error; + `; - // invalid param that should throw - expect(() => { - getContext(['class'], scriptTag); - }).toThrow('getContext: JavaScript keywords are not allowed in evaluate array'); + const consoleError = jest.spyOn(console, 'error'); expect(() => { - getContext(['const'], scriptTag); - }).toThrow('getContext: JavaScript keywords are not allowed in evaluate array'); + const context = getContext(['valid', 'invalid'], scriptTag); + expect(consoleError).toHaveBeenCalledWith("getContext: error evaluating 'valid'"); + expect(consoleError).toHaveBeenCalledWith("getContext: error evaluating 'invalid'"); + expect(context).toStrictEqual({}); + }).not.toThrow(); + + consoleError.mockRestore(); + }); + + it('does not throw an error when keywords are provided in the evaluate array, but logs an error', () => { + const scriptTag = document.createElement('script'); + scriptTag.setAttribute('type', 'searchspring'); + + const consoleError = jest.spyOn(console, 'error'); expect(() => { - getContext(['if'], scriptTag); - }).toThrow('getContext: JavaScript keywords are not allowed in evaluate array'); + // invalid param that should generate an error - `getContext: JavaScript keyword found: '${item}'! Please use a different variable name.` + getContext(['class', 'const', 'if', 'valid'], scriptTag); + expect(consoleError).toHaveBeenCalledWith("getContext: JavaScript keyword found: 'class'! Please use a different variable name."); + expect(consoleError).toHaveBeenCalledWith("getContext: JavaScript keyword found: 'const'! Please use a different variable name."); + expect(consoleError).toHaveBeenCalledWith("getContext: JavaScript keyword found: 'if'! Please use a different variable name."); + expect(consoleError).not.toHaveBeenCalledWith("getContext: JavaScript keyword found: 'valid'! Please use a different variable name."); + + expect(consoleError).toHaveBeenCalledTimes(3); + }).not.toThrow(); + + consoleError.mockRestore(); }); - it('throws an error when JavaScript keywords are found in script inner variables', () => { + it('does not throw when keywords are using in inner script variables but logs an error and returns an empty context', () => { const scriptTag = document.createElement('script'); scriptTag.setAttribute('type', 'searchspring'); scriptTag.innerHTML = ` @@ -329,9 +351,45 @@ describe('getContext', () => { validVar = "should-evaluate"; `; + const consoleError = jest.spyOn(console, 'error'); + expect(() => { - getContext(['validVar'], scriptTag); - }).toThrow('getContext: JavaScript keywords cannot be used as variable names in script'); + const context = getContext(['validVar'], scriptTag); + + expect(consoleError).toHaveBeenCalledWith("getContext: JavaScript keyword found: 'class'! Please use a different variable name."); + expect(consoleError).toHaveBeenCalledWith("getContext: JavaScript keyword found: 'const'! Please use a different variable name."); + expect(consoleError).toHaveBeenCalledWith("getContext: JavaScript keyword found: 'if'! Please use a different variable name."); + expect(consoleError).not.toHaveBeenCalledWith("getContext: JavaScript keyword found: 'validVar'! Please use a different variable name."); + + expect(consoleError).toHaveBeenCalledWith("getContext: error evaluating 'validVar'"); + + // logs above errors plus the actual error when attempting to evaluate "validVar" + expect(consoleError).toHaveBeenCalledTimes(5); + + expect(context).toStrictEqual({}); + }).not.toThrow(); + + consoleError.mockRestore(); + }); + + it('allows javascript keywords in object properties and string values', () => { + const scriptTag = document.createElement('script'); + scriptTag.setAttribute('type', 'searchspring'); + scriptTag.innerHTML = ` + config = { + class: "class", + const: "const", + if: true + }; + `; + + const vars = getContext(['config'], scriptTag); + expect(vars).toHaveProperty('config'); + expect(vars.config).toEqual({ + class: 'class', + const: 'const', + if: true, + }); }); }); @@ -402,40 +460,3 @@ describe('variable name parsing', () => { expect(vars).not.toHaveProperty('nested'); }); }); - -describe('javascript keywords', () => { - it('filters out javascript keywords from evaluation', () => { - const scriptTag = document.createElement('script'); - scriptTag.setAttribute('type', 'searchspring'); - scriptTag.innerHTML = ` - class = "should-not-evaluate"; - const = "should-not-evaluate"; - if = "should-not-evaluate"; - validVar = "should-evaluate"; - `; - - expect(() => { - const vars = getContext(['class', 'const', 'if', 'validVar'], scriptTag); - }).toThrow('getContext: JavaScript keywords are not allowed in evaluate array'); - }); - - it('allows javascript keywords in object properties and string values', () => { - const scriptTag = document.createElement('script'); - scriptTag.setAttribute('type', 'searchspring'); - scriptTag.innerHTML = ` - config = { - class: "my-class", - const: "my-const", - if: true - }; - `; - - const vars = getContext(['config'], scriptTag); - expect(vars).toHaveProperty('config'); - expect(vars.config).toEqual({ - class: 'my-class', - const: 'my-const', - if: true, - }); - }); -}); diff --git a/packages/snap-toolbox/src/getContext/getContext.ts b/packages/snap-toolbox/src/getContext/getContext.ts index a6eba1c1a..8ab51acb0 100644 --- a/packages/snap-toolbox/src/getContext/getContext.ts +++ b/packages/snap-toolbox/src/getContext/getContext.ts @@ -49,10 +49,6 @@ const JAVASCRIPT_KEYWORDS = new Set([ ]); export function getContext(evaluate: string[] = [], script?: HTMLScriptElement | string): ContextVariables { - if (evaluate?.some((name) => JAVASCRIPT_KEYWORDS.has(name))) { - throw new Error('getContext: JavaScript keywords are not allowed in evaluate array'); - } - if (!script || typeof script === 'string') { const scripts = Array.from(document.querySelectorAll((script as string) || 'script[id^=searchspring], script[src*="snapui.searchspring.io"]')); @@ -106,15 +102,16 @@ export function getContext(evaluate: string[] = [], script?: HTMLScriptElement | .match(/([a-zA-Z_$][a-zA-Z_$0-9]*)\s*=/g) ?.map((match) => match.replace(/[\s=]/g, '')); - if (scriptInnerVars?.some((name) => JAVASCRIPT_KEYWORDS.has(name))) { - throw new Error('getContext: JavaScript keywords cannot be used as variable names in script'); - } - const combinedVars = evaluate.concat(scriptInnerVars || []); // de-dupe vars const evaluateVars = combinedVars.filter((item, index) => { - return combinedVars.indexOf(item) === index && !JAVASCRIPT_KEYWORDS.has(item); + const isKeyword = JAVASCRIPT_KEYWORDS.has(item); + // console error if keyword + if (isKeyword) { + console.error(`getContext: JavaScript keyword found: '${item}'! Please use a different variable name.`); + } + return combinedVars.indexOf(item) === index && !isKeyword; }); // evaluate text and put into variables @@ -126,9 +123,13 @@ export function getContext(evaluate: string[] = [], script?: HTMLScriptElement | return ${name}; `); scriptVariables[name] = fn(); - } catch (e) { + } catch (err) { // if evaluation fails, set to undefined - console.log(`getContext: error evaluating ${name}`); + const isKeyword = JAVASCRIPT_KEYWORDS.has(name); + if (!isKeyword) { + console.error(`getContext: error evaluating '${name}'`); + console.error(err); + } scriptVariables[name] = undefined; } });