Skip to content

Commit 9bb7d9b

Browse files
TrySoundTrySound
committed
fix: rebind variables within scope (#4927)
Found broken cases with rebinding variables when they are renamed or just saved.
1 parent 84b5036 commit 9bb7d9b

File tree

2 files changed

+152
-39
lines changed

2 files changed

+152
-39
lines changed

‎apps/builder/app/shared/data-variables.test.tsx

+105-6
Original file line numberDiff line numberDiff line change
@@ -262,7 +262,11 @@ test("restore tree variables in children", () => {
262262
</$.Box>
263263
</$.Body>
264264
);
265-
rebindTreeVariablesMutable({ startingInstanceId: "boxId", ...data });
265+
rebindTreeVariablesMutable({
266+
startingInstanceId: "boxId",
267+
pages: createDefaultPages({ rootInstanceId: "bodyId" }),
268+
...data,
269+
});
266270
expect(Array.from(data.dataSources.values())).toEqual([
267271
expect.objectContaining({ scopeInstanceId: "bodyId" }),
268272
expect.objectContaining({ scopeInstanceId: "boxId" }),
@@ -290,7 +294,11 @@ test("restore tree variables in props", () => {
290294
</$.Box>
291295
</$.Body>
292296
);
293-
rebindTreeVariablesMutable({ startingInstanceId: "boxId", ...data });
297+
rebindTreeVariablesMutable({
298+
startingInstanceId: "boxId",
299+
pages: createDefaultPages({ rootInstanceId: "bodyId" }),
300+
...data,
301+
});
294302
const [_bodyVariableId, boxOneVariableId, boxTwoVariableId] =
295303
data.dataSources.keys();
296304
const boxOneIdentifier = encodeDataVariableId(boxOneVariableId);
@@ -337,7 +345,11 @@ test("rebind tree variables in props and children", () => {
337345
</$.Box>
338346
</$.Body>
339347
);
340-
rebindTreeVariablesMutable({ startingInstanceId: "boxId", ...data });
348+
rebindTreeVariablesMutable({
349+
startingInstanceId: "boxId",
350+
pages: createDefaultPages({ rootInstanceId: "bodyId" }),
351+
...data,
352+
});
341353
expect(Array.from(data.dataSources.values())).toEqual([
342354
expect.objectContaining({ scopeInstanceId: "bodyId" }),
343355
expect.objectContaining({ scopeInstanceId: "boxId" }),
@@ -358,6 +370,32 @@ test("rebind tree variables in props and children", () => {
358370
]);
359371
});
360372

373+
test("preserve nested variables with the same name when rebind", () => {
374+
const bodyVariable = new Variable("one", "one value of body");
375+
const textVariable = new Variable("one", "one value of box");
376+
const data = renderData(
377+
<$.Body ws:id="bodyId" data-body-vars={expression`${bodyVariable}`}>
378+
<$.Text ws:id="textId" data-text-vars={expression`${textVariable}`}>
379+
{expression`${textVariable}`}
380+
</$.Text>
381+
</$.Body>
382+
);
383+
rebindTreeVariablesMutable({
384+
startingInstanceId: "bodyId",
385+
pages: createDefaultPages({ rootInstanceId: "bodyId" }),
386+
...data,
387+
});
388+
expect(Array.from(data.dataSources.values())).toEqual([
389+
expect.objectContaining({ scopeInstanceId: "bodyId" }),
390+
expect.objectContaining({ scopeInstanceId: "textId" }),
391+
]);
392+
const [_bodyVariableId, textVariableId] = data.dataSources.keys();
393+
const textIdentifier = encodeDataVariableId(textVariableId);
394+
expect(data.instances.get("textId")?.children).toEqual([
395+
{ type: "expression", value: textIdentifier },
396+
]);
397+
});
398+
361399
test("restore tree variables in resources", () => {
362400
const bodyVariable = new Variable("one", "one value of body");
363401
const boxVariable = new Variable("one", "one value of box");
@@ -384,7 +422,11 @@ test("restore tree variables in resources", () => {
384422
</$.Box>
385423
</$.Body>
386424
);
387-
rebindTreeVariablesMutable({ startingInstanceId: "boxId", ...data });
425+
rebindTreeVariablesMutable({
426+
startingInstanceId: "boxId",
427+
pages: createDefaultPages({ rootInstanceId: "bodyId" }),
428+
...data,
429+
});
388430
expect(Array.from(data.dataSources.values())).toEqual([
389431
expect.objectContaining({ scopeInstanceId: "bodyId" }),
390432
expect.objectContaining({ scopeInstanceId: "boxId" }),
@@ -434,7 +476,11 @@ test("rebind tree variables in resources", () => {
434476
</$.Box>
435477
</$.Body>
436478
);
437-
rebindTreeVariablesMutable({ startingInstanceId: "boxId", ...data });
479+
rebindTreeVariablesMutable({
480+
startingInstanceId: "boxId",
481+
pages: createDefaultPages({ rootInstanceId: "bodyId" }),
482+
...data,
483+
});
438484
expect(Array.from(data.dataSources.values())).toEqual([
439485
expect.objectContaining({ scopeInstanceId: "bodyId" }),
440486
expect.objectContaining({ scopeInstanceId: "boxId" }),
@@ -458,6 +504,55 @@ test("rebind tree variables in resources", () => {
458504
]);
459505
});
460506

507+
test("rebind global variables in resources", () => {
508+
const globalVariable = new Variable("globalVariable", "");
509+
const data = renderData(
510+
<ws.root ws:id={ROOT_INSTANCE_ID} data-vars={expression`${globalVariable}`}>
511+
<$.Body ws:id="bodyId">
512+
<$.Text ws:id="textId">{expression`globalVariable`}</$.Text>
513+
</$.Body>
514+
</ws.root>
515+
);
516+
data.instances.delete(ROOT_INSTANCE_ID);
517+
rebindTreeVariablesMutable({
518+
startingInstanceId: ROOT_INSTANCE_ID,
519+
pages: createDefaultPages({ rootInstanceId: "bodyId" }),
520+
...data,
521+
});
522+
expect(Array.from(data.dataSources.values())).toEqual([
523+
expect.objectContaining({ scopeInstanceId: ROOT_INSTANCE_ID }),
524+
]);
525+
const [globalVariableId] = data.dataSources.keys();
526+
const globalIdentifier = encodeDataVariableId(globalVariableId);
527+
expect(data.instances.get("textId")?.children).toEqual([
528+
{ type: "expression", value: globalIdentifier },
529+
]);
530+
});
531+
532+
test("preserve other variables when rebind", () => {
533+
const bodyVariable = new Variable("globalVariable", "");
534+
const textVariable = new Variable("textVariable", "");
535+
const data = renderData(
536+
<$.Body ws:id="bodyId" data-vars={expression`${bodyVariable}`}>
537+
<$.Text ws:id="textId">{expression`${textVariable}`}</$.Text>
538+
</$.Body>
539+
);
540+
rebindTreeVariablesMutable({
541+
startingInstanceId: "bodyId",
542+
pages: createDefaultPages({ rootInstanceId: "bodyId" }),
543+
...data,
544+
});
545+
expect(Array.from(data.dataSources.values())).toEqual([
546+
expect.objectContaining({ scopeInstanceId: "bodyId" }),
547+
expect.objectContaining({ scopeInstanceId: "textId" }),
548+
]);
549+
const [_globalVariableId, textVariableId] = data.dataSources.keys();
550+
const textIdentifier = encodeDataVariableId(textVariableId);
551+
expect(data.instances.get("textId")?.children).toEqual([
552+
{ type: "expression", value: textIdentifier },
553+
]);
554+
});
555+
461556
test("prevent rebinding tree variables from slots", () => {
462557
const bodyVariable = new Variable("myVariable", "one value of body");
463558
const data = renderData(
@@ -469,7 +564,11 @@ test("prevent rebinding tree variables from slots", () => {
469564
</$.Slot>
470565
</$.Body>
471566
);
472-
rebindTreeVariablesMutable({ startingInstanceId: "boxId", ...data });
567+
rebindTreeVariablesMutable({
568+
startingInstanceId: "boxId",
569+
pages: createDefaultPages({ rootInstanceId: "bodyId" }),
570+
...data,
571+
});
473572
expect(data.instances.get("boxId")?.children).toEqual([
474573
{ type: "expression", value: "myVariable" },
475574
]);

‎apps/builder/app/shared/data-variables.ts

+47-33
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,11 @@ const traverseExpressions = ({
275275
props: Props;
276276
dataSources: DataSources;
277277
resources: Resources;
278-
update: (expression: string, args?: string[]) => void | string;
278+
update: (
279+
expression: string,
280+
instanceId: Instance["id"],
281+
args?: string[]
282+
) => void | string;
279283
}) => {
280284
const pagesList = pages ? [pages.homePage, ...pages.pages] : [];
281285

@@ -297,45 +301,51 @@ const traverseExpressions = ({
297301
startingInstanceId === page.rootInstanceId ||
298302
startingInstanceId === ROOT_INSTANCE_ID
299303
) {
300-
page.title = update(page.title) ?? page.title;
304+
const { rootInstanceId } = page;
305+
page.title = update(page.title, rootInstanceId) ?? page.title;
301306
if (page.meta.description) {
302307
page.meta.description =
303-
update(page.meta.description) ?? page.meta.description;
308+
update(page.meta.description, rootInstanceId) ??
309+
page.meta.description;
304310
}
305311
if (page.meta.excludePageFromSearch) {
306312
page.meta.excludePageFromSearch =
307-
update(page.meta.excludePageFromSearch) ??
313+
update(page.meta.excludePageFromSearch, rootInstanceId) ??
308314
page.meta.excludePageFromSearch;
309315
}
310316
if (page.meta.socialImageUrl) {
311317
page.meta.socialImageUrl =
312-
update(page.meta.socialImageUrl) ?? page.meta.socialImageUrl;
318+
update(page.meta.socialImageUrl, rootInstanceId) ??
319+
page.meta.socialImageUrl;
313320
}
314321
if (page.meta.language) {
315-
page.meta.language = update(page.meta.language) ?? page.meta.language;
322+
page.meta.language =
323+
update(page.meta.language, rootInstanceId) ?? page.meta.language;
316324
}
317325
if (page.meta.status) {
318-
page.meta.status = update(page.meta.status) ?? page.meta.status;
326+
page.meta.status =
327+
update(page.meta.status, rootInstanceId) ?? page.meta.status;
319328
}
320329
if (page.meta.redirect) {
321-
page.meta.redirect = update(page.meta.redirect) ?? page.meta.redirect;
330+
page.meta.redirect =
331+
update(page.meta.redirect, rootInstanceId) ?? page.meta.redirect;
322332
}
323333
if (page.meta.custom) {
324334
for (const item of page.meta.custom) {
325-
item.content = update(item.content) ?? item.content;
335+
item.content = update(item.content, rootInstanceId) ?? item.content;
326336
}
327337
}
328338
}
329339
}
330-
const resourceIds = new Set<Resource["id"]>();
340+
const instanceIdByResourceId = new Map<Resource["id"], Instance["id"]>();
331341

332342
for (const instance of instances.values()) {
333343
if (instanceIds.has(instance.id) === false) {
334344
continue;
335345
}
336346
for (const child of instance.children) {
337347
if (child.type === "expression") {
338-
child.value = update(child.value) ?? child.value;
348+
child.value = update(child.value, instance.id) ?? child.value;
339349
}
340350
}
341351
}
@@ -345,40 +355,40 @@ const traverseExpressions = ({
345355
continue;
346356
}
347357
if (prop.type === "expression") {
348-
prop.value = update(prop.value) ?? prop.value;
358+
prop.value = update(prop.value, prop.instanceId) ?? prop.value;
349359
continue;
350360
}
351361
if (prop.type === "action") {
352362
for (const action of prop.value) {
353-
action.code = update(action.code, action.args) ?? action.code;
363+
action.code =
364+
update(action.code, prop.instanceId, action.args) ?? action.code;
354365
}
355366
continue;
356367
}
357368
if (prop.type === "resource") {
358-
resourceIds.add(prop.value);
369+
instanceIdByResourceId.set(prop.value, prop.instanceId);
359370
continue;
360371
}
361372
}
362373

363374
for (const dataSource of dataSources.values()) {
364-
if (
365-
instanceIds.has(dataSource.scopeInstanceId ?? "") &&
366-
dataSource.type === "resource"
367-
) {
368-
resourceIds.add(dataSource.resourceId);
375+
const instanceId = dataSource.scopeInstanceId ?? "";
376+
if (instanceIds.has(instanceId) && dataSource.type === "resource") {
377+
instanceIdByResourceId.set(dataSource.resourceId, instanceId);
369378
}
370379
}
371380

372381
for (const resource of resources.values()) {
373-
if (resourceIds.has(resource.id) === false) {
382+
const instanceId = instanceIdByResourceId.get(resource.id);
383+
if (instanceId === undefined) {
374384
continue;
375385
}
376-
resource.url = update(resource.url) ?? resource.url;
386+
resource.url = update(resource.url, instanceId) ?? resource.url;
377387
for (const header of resource.headers) {
378-
header.value = update(header.value) ?? header.value;
388+
header.value = update(header.value, instanceId) ?? header.value;
379389
}
380390
if (resource.body) {
381-
resource.body = update(resource.body) ?? resource.body;
391+
resource.body = update(resource.body, instanceId) ?? resource.body;
382392
}
383393
}
384394
};
@@ -404,7 +414,7 @@ export const findUnsetVariableNames = ({
404414
props,
405415
dataSources,
406416
resources,
407-
update: (expression, args = []) => {
417+
update: (expression, _instanceId, args = []) => {
408418
transpileExpression({
409419
expression,
410420
replaceVariable: (identifier) => {
@@ -458,34 +468,38 @@ export const findUsedVariables = ({
458468

459469
export const rebindTreeVariablesMutable = ({
460470
startingInstanceId,
471+
pages,
461472
instances,
462473
props,
463474
dataSources,
464475
resources,
465476
}: {
466477
startingInstanceId: Instance["id"];
478+
pages: undefined | Pages;
467479
instances: Instances;
468480
props: Props;
469481
dataSources: DataSources;
470482
resources: Resources;
471483
}) => {
472-
const maskedVariables = findMaskedVariablesByInstanceId({
473-
startingInstanceId,
474-
dataSources,
475-
instances,
476-
});
484+
// unset all variables
477485
const unsetNameById = new Map<DataSource["id"], DataSource["name"]>();
478-
for (const { id, name } of dataSources.values()) {
479-
unsetNameById.set(id, name);
486+
for (const dataSource of dataSources.values()) {
487+
unsetNameById.set(dataSource.id, dataSource.name);
480488
}
481489
traverseExpressions({
482490
startingInstanceId,
483-
pages: undefined,
491+
pages,
484492
instances,
485493
props,
486494
dataSources,
487495
resources,
488-
update: (expression, args) => {
496+
update: (expression, instanceId, args) => {
497+
// restore all masked variables of current scope
498+
const maskedVariables = findMaskedVariablesByInstanceId({
499+
startingInstanceId: instanceId,
500+
dataSources,
501+
instances,
502+
});
489503
let maskedIdByName = new Map(maskedVariables);
490504
if (args) {
491505
maskedIdByName = new Map(maskedIdByName);

0 commit comments

Comments
 (0)