From 281d7fb474f4f17bb75a794ea6046843b0872a57 Mon Sep 17 00:00:00 2001 From: James Yang Date: Tue, 19 Mar 2024 11:18:13 -0400 Subject: [PATCH] Fix bug where aliases to remote variables could not be updated when syncing tokens to Figma (#23) For the `sync_tokens_to_figma.ts` script, if a tokens file has aliases to variables in remote collections, e.g. `"$value": "{name_of_variable}"`, update those alias references when needed. This issue was brought up in https://github.com/figma/variables-github-action-example/pull/20. The previous behavior was that if a tokens file has aliases to variables in a remote collection, those aliases would cause a 400 response from the `POST /v1/files/:file_key/variables` endpoint, as the script was sending up invalid variable ids inside the alias objects. Now, the script will match up token values with remote variable names and update aliases when needed. Note: we are limited by the variables API in that we cannot alias variables that aren't already consumed by current file. Demo: Here is a demo of the `sync_tokens_to_figma.ts` script updating a Figma file that has a Semantic variables collection with aliases to variables published from another file. https://github.com/figma/variables-github-action-example/assets/250513/592fddbc-38fe-485c-89ca-688b1777b64f ## Test Plan - In one file, create and publish a variable collection - In another file, create a variable collection with variables that alias to variables from the file above - Run `npm run sync-figma-to-tokens` to export the variables from the second file into a tokens file - Run `npm run sync-tokens-to-figma` on the exported tokens file to sync the tokens back to Figma. The result is that we should noop instead of getting a 400 error from the REST API. --- src/token_import.test.ts | 203 ++++++++++++++++++++++++++++++++++++--- src/token_import.ts | 16 ++- 2 files changed, 198 insertions(+), 21 deletions(-) diff --git a/src/token_import.test.ts b/src/token_import.test.ts index d1b2a87..c2a398a 100644 --- a/src/token_import.test.ts +++ b/src/token_import.test.ts @@ -629,7 +629,7 @@ describe('generatePostVariablesPayload', () => { }) }) - it('ignores remote collections and variables', () => { + it('noops if tokens happen to match remote collections and variables', () => { const localVariablesResponse: GetLocalVariablesResponse = { status: 200, error: false, @@ -712,17 +712,198 @@ describe('generatePostVariablesPayload', () => { const result = generatePostVariablesPayload(tokensByFile, localVariablesResponse) - // Since all existing collections and variables are remote, result should be equivalent to an initial sync - expect(result).toEqual( - generatePostVariablesPayload(tokensByFile, { - status: 200, - error: false, - meta: { - variableCollections: {}, - variables: {}, + expect(result).toEqual({ + variableCollections: [], + variableModes: [], + variables: [], + variableModeValues: [], + }) + }) + + it('throws on attempted modifications to remote variables', () => { + const localVariablesResponse: GetLocalVariablesResponse = { + status: 200, + error: false, + meta: { + variableCollections: { + 'VariableCollectionId:1:1': { + id: 'VariableCollectionId:1:1', + name: 'collection', + modes: [{ modeId: '1:0', name: 'mode1' }], + defaultModeId: '1:0', + remote: true, + key: 'variableKey', + hiddenFromPublishing: false, + variableIds: ['VariableID:2:1', 'VariableID:2:2'], + }, }, - }), - ) + variables: { + 'VariableID:2:1': { + id: 'VariableID:2:1', + name: 'var1', + key: 'variable_key', + variableCollectionId: 'VariableCollectionId:1:1', + resolvedType: 'STRING', + valuesByMode: { + '1:0': 'hello world!', + }, + remote: true, + description: '', + hiddenFromPublishing: false, + scopes: ['ALL_SCOPES'], + codeSyntax: {}, + }, + 'VariableID:2:2': { + id: 'VariableID:2:2', + name: 'var2', + key: 'variable_key2', + variableCollectionId: 'VariableCollectionId:1:1', + resolvedType: 'STRING', + valuesByMode: { + '1:0': { type: 'VARIABLE_ALIAS', id: 'VariableID:2:1' }, + }, + remote: true, + description: '', + hiddenFromPublishing: false, + scopes: ['ALL_SCOPES'], + codeSyntax: {}, + }, + }, + }, + } + + const tokensByFile: FlattenedTokensByFile = { + 'collection.mode1.json': { + var1: { + $type: 'string', + $value: 'hello world!', + $description: '', + $extensions: { + 'com.figma': { + hiddenFromPublishing: true, // modification + scopes: ['ALL_SCOPES'], + codeSyntax: {}, + }, + }, + }, + var2: { + $type: 'string', + $value: '{var1}', + $description: '', + $extensions: { + 'com.figma': { + hiddenFromPublishing: false, + scopes: ['ALL_SCOPES'], + codeSyntax: {}, + }, + }, + }, + }, + } + + expect(() => { + generatePostVariablesPayload(tokensByFile, localVariablesResponse) + }).toThrowError(`Cannot update remote variable "var1" in collection "collection"`) + }) + + it('updates aliases to remote variables', () => { + const localVariablesResponse: GetLocalVariablesResponse = { + status: 200, + error: false, + meta: { + variableCollections: { + 'VariableCollectionId:1:1': { + id: 'VariableCollectionId:1:1', + name: 'primitives', + modes: [{ modeId: '1:0', name: 'mode1' }], + defaultModeId: '1:0', + remote: true, + key: 'variableCollectionKey1', + hiddenFromPublishing: false, + variableIds: ['VariableID:1:2', 'VariableID:1:3'], + }, + 'VariableCollectionId:2:1': { + id: 'VariableCollectionId:2:1', + name: 'tokens', + modes: [{ modeId: '2:0', name: 'mode1' }], + defaultModeId: '2:0', + remote: false, + key: 'variableCollectionKey2', + hiddenFromPublishing: false, + variableIds: ['VariableID:2:1'], + }, + }, + variables: { + // 2 color variables in the primitives collection + 'VariableID:1:2': { + id: 'VariableID:1:2', + name: 'gray/100', + key: 'variableKey1', + variableCollectionId: 'VariableCollectionId:1:1', + resolvedType: 'COLOR', + valuesByMode: { + '1:0': { r: 0.98, g: 0.98, b: 0.98, a: 1 }, + }, + remote: true, + description: 'light gray', + hiddenFromPublishing: false, + scopes: ['ALL_SCOPES'], + codeSyntax: {}, + }, + 'VariableID:1:3': { + id: 'VariableID:1:3', + name: 'gray/200', + key: 'variableKey2', + variableCollectionId: 'VariableCollectionId:1:1', + resolvedType: 'COLOR', + valuesByMode: { + '1:0': { r: 0.96, g: 0.96, b: 0.96, a: 1 }, + }, + remote: true, + description: 'light gray', + hiddenFromPublishing: false, + scopes: ['ALL_SCOPES'], + codeSyntax: {}, + }, + // 1 color variable in the tokens collection + 'VariableID:2:1': { + id: 'VariableID:2:1', + name: 'surface/surface-brand', + key: 'variableKey3', + variableCollectionId: 'VariableCollectionId:2:1', + resolvedType: 'COLOR', + valuesByMode: { + '2:0': { type: 'VARIABLE_ALIAS', id: 'VariableID:1:2' }, + }, + remote: false, + description: 'light gray', + hiddenFromPublishing: false, + scopes: ['ALL_SCOPES'], + codeSyntax: {}, + }, + }, + }, + } + + const tokensByFile: FlattenedTokensByFile = { + 'tokens.mode1.json': { + // Change the alias to point to the other variable in the primitives collection + 'surface/surface-brand': { $type: 'color', $value: '{gray.200}' }, + }, + } + + const result = generatePostVariablesPayload(tokensByFile, localVariablesResponse) + + expect(result.variableCollections).toEqual([]) + expect(result.variableModes).toEqual([]) + expect(result.variables).toEqual([]) + expect(result.variableModeValues).toEqual([ + { + variableId: 'VariableID:2:1', + modeId: '2:0', + value: { type: 'VARIABLE_ALIAS', id: 'VariableID:1:3' }, + }, + ]) }) it('throws on unsupported token types', async () => { diff --git a/src/token_import.ts b/src/token_import.ts index 47d057f..3fc5144 100644 --- a/src/token_import.ts +++ b/src/token_import.ts @@ -242,11 +242,6 @@ export function generatePostVariablesPayload( } = {} Object.values(localVariables.meta.variableCollections).forEach((collection) => { - // Skip over remote collections because we can't modify them - if (collection.remote) { - return - } - if (localVariableCollectionsByName[collection.name]) { throw new Error(`Duplicate variable collection in file: ${collection.name}`) } @@ -255,11 +250,6 @@ export function generatePostVariablesPayload( }) Object.values(localVariables.meta.variables).forEach((variable) => { - // Skip over remote variables because we can't modify them - if (variable.remote) { - return - } - if (!localVariablesByCollectionAndName[variable.variableCollectionId]) { localVariablesByCollectionAndName[variable.variableCollectionId] = {} } @@ -350,6 +340,12 @@ export function generatePostVariablesPayload( ...differences, }) } else if (variable && Object.keys(differences).length > 0) { + if (variable.remote) { + throw new Error( + `Cannot update remote variable "${variable.name}" in collection "${collectionName}"`, + ) + } + postVariablesPayload.variables!.push({ action: 'UPDATE', id: variableId,