Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

usePrototypeForDefaults cause issues with repeated fields #498

Open
alexeyten opened this issue Feb 2, 2022 · 4 comments · May be fixed by #499
Open

usePrototypeForDefaults cause issues with repeated fields #498

alexeyten opened this issue Feb 2, 2022 · 4 comments · May be fixed by #499

Comments

@alexeyten
Copy link
Contributor

For this proto

message Test {
  repeated string tags = 1;
}

the generated code is

function createBaseTest() {
  return { tags: [] };
}

const message = Object.create(createBaseTest());
// ...
case 1:
  message.tags.push(reader.string());

and this will push tags into prototype's tags field. And, e.g. console.log(message) will show an “empty” object and Object.keys().includes("tags") will be false.

@stephenh
Copy link
Owner

stephenh commented Feb 2, 2022

Ah shoot, right, we forgot we'll have to make new instances of lists and maps... @Vilsol would you like to take a look at ^?

@stephenh
Copy link
Owner

stephenh commented Feb 2, 2022

So when message.tags is actually in message, it's pretty clear we should create a new message.tags: [] on the instance and push to that.

What's interesting is, what should we do if message.tags is not in the message? E.g. we either do:

  1. Make createBaseTest return a frozen [] so that callers can continue to do a hazzer check on whether message.tags is the default list or not, but cannot write to it.
  2. Always create an empty list on message.tags, to allow callers to write/mutate it w/o changing every message instance, but then they also loose the ability to do the hazzer check of "is tags from your prototype"?

@alexeyten
Copy link
Contributor Author

I've created this proof of concept for “magic” repeated field:

/**
 * Proof of concept of “magic” repeated field.
 * 
 * message Test {
 *   uint32 id = 1;
 *   repeated string tags = 2;
 *   string last = 3;
 * }
 */

interface Test {
    id: number;
    tags: string[];
    last: string;
}

const TestBase: Test = {
    id:  0,
    tags: null as unknown as string[],
    last: '',
};

Object.defineProperty(TestBase, 'tags', {
    configurable: false,
    get() {
        const value: string[] = [];
        if (this === TestBase) return value;
        Object.defineProperty(this, 'tags', {
            enumerable: true,
            configurable: true,
            writable: true,
            value
        });
        return value;
    },
    set(value: string[]) {
        Object.defineProperty(this, 'tags', {
            enumerable: true,
            configurable: true,
            writable: true,
            value
        });
    }
});

console.log('TestBase', TestBase);
const message = Object.create(TestBase) as Test;

console.log('Empty message', message);
console.log('Empty message keys', Object.keys(message));

message.id = 42;
// tags is populated automatically
message.tags.push('test', 'my', 'tags');
message.last = 'Final';
console.log('Filled message', message);
console.log('TestBase', TestBase);

Playground Link: Provided

@alexeyten alexeyten linked a pull request Feb 4, 2022 that will close this issue
@alexeyten
Copy link
Contributor Author

alexeyten commented Feb 5, 2022

Another version of “magic”. With helper functions

/**
 * Proof of concept of “magic” repeated field.
 * 
 * message Test {
 *   uint32 id = 1;
 *   repeated string tags = 2;
 *   repeated uint32 children = 3;
 * }
 * 
 * message Second {
 *   repeated Test tests = 1;
 * }
 */

interface Test {
    id: number;
    tags: string[];
    children: number[];
}

interface Second {
    tests: Test[];
}

function materialize<Base>(obj: Base, field: keyof Base, value: unknown[]) {
    Object.defineProperty(obj, field, {
        enumerable: true,
        configurable: true,
        writable: true,
        value
    });
    return value;
};

function createRepeatedFields<Base>(Base: Base, ...fields: Array<keyof Base>) {
    fields.forEach(field => {
        Object.defineProperty(Base, field, {
            enumerable: true,
            get() {
                const value: unknown[] = [];
                if (this === Base) return value;
                return materialize(this, field, value);
            },
            set(value: unknown[]) {
                materialize(this, field, value);
            }
        });
    });

    return Base;
}

const TestBase: Test = createRepeatedFields({ id:  0 } as Test, 'tags', 'children');
const SecondBase: Second = createRepeatedFields({} as Second, 'tests');

console.log('TestBase', TestBase);
console.log('SecondBase', SecondBase);

const test = Object.create(TestBase) as Test;
const second: Second = Object.create(SecondBase);

console.log('Fresh test and keys', test, Object.keys(test));
console.log('Fresh second and keys', second, Object.keys(second));

test.id = 42;
// tags is populated automatically
test.tags.push('test', 'my', 'tags');
// one could assign array directly
test.children = [ 1, 3 ];
console.log('Filled test and keys', test, Object.keys(test));

second.tests.push(test, test);
console.log('Filled second and keys', second, Object.keys(second));

console.log('TestBase still empty', TestBase);
console.log('SecondBase still empty', SecondBase);

Playground Link: Provided

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants