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

feat: add usePrototypeForDefaults option #484

Merged
merged 3 commits into from
Jan 20, 2022

Conversation

Vilsol
Copy link
Collaborator

@Vilsol Vilsol commented Jan 20, 2022

This adds a feature flag usePrototypeForDefaults as discussed in #479

I have tested this by building ts-proto with this change, rebuilding the ts-proto-descriptors lib using the rebuilt ts-proto, then linking to the new ts-proto-descriptors from ts-proto and re-building all integrations and testing them.

Copy link
Owner

@stephenh stephenh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

import { Child_Type, Simple, StateEnum } from './simple';

describe('simple', () => {
it('generates types correctly', () => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a test that does a Simple.decode that shows a from-default value like age: 0 has Object.keys().includes("age") === false or what not?

let createBase = code`createBase${fullName}()`;
if (options.usePrototypeForDefaults) {
createBase = code`Object.create(${createBase}) as ${fullName}`;
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah wow, nice how small this change is.

@Vilsol
Copy link
Collaborator Author

Vilsol commented Jan 20, 2022

@stephenh I added some more appropriate tests. I hope that is what you meant.

@stephenh
Copy link
Owner

I added some more appropriate tests. I hope that is what you meant.

Yep, that's what I meant. Thanks!

})).finish());

expect(Object.keys(decodedWithZero).includes('age')).toBeFalsy();
expect(decodedWithZero.hasOwnProperty('age')).toBeFalsy();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right right, I'd expected this to be truthy, but it's the whole proto3 thing that optionals are not encoded, so presence is broken. But this will work for proto2, which is what the descriptors comein as.

@stephenh stephenh merged commit 8e8c810 into stephenh:main Jan 20, 2022
stephenh pushed a commit that referenced this pull request Jan 20, 2022
# [1.103.0](v1.102.2...v1.103.0) (2022-01-20)

### Features

* add usePrototypeForDefaults option ([#484](#484)) ([8e8c810](8e8c810))
@stephenh
Copy link
Owner

🎉 This PR is included in version 1.103.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

2 participants