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

Solution for the issue of snake_case fields being lost after calling the create method #1001

Closed
wants to merge 1 commit into from

Conversation

locene
Copy link

@locene locene commented Oct 29, 2024

Description

Given the following data:

const data = {
    "sample_content": "some things"
}

And the corresponding proto definition:

message Data {
    string sample_content = 1;
}

When using this setup in the create method:

create(DataSchema, {
    data: data
})

The "sample_content" field will be ignored and not converted correctly.

Analysis

In the initMessage method of create.ts, the value is accessed using member.localName:

let value = (init as Record<string, unknown>)[member.localName]

At this point, the value of member.localName has already been converted to camelCase, resulting in "sampleContent", which does not match the original snake_case format of "sample_content", causing this field to be ignored.

Solution

To address this issue, modify the code as follows:

let value = (init as Record<string, unknown>)[member.localName] ?? (init as Record<string, unknown>)[member.name] ?? null;

This change will resolve the issue without impacting the existing functionality.

@CLAassistant
Copy link

CLAassistant commented Oct 29, 2024

CLA assistant check
All committers have signed the CLA.

@locene locene marked this pull request as ready for review October 29, 2024 13:41
@timostamm
Copy link
Member

From the manual:

Property names are always lowerCamelCase, even if the corresponding Protobuf field uses snake_case. Though there's no official style for ECMAScript, most style guides (AirBnB, MDN, Google) as well as Node.js APIs and browser APIs use lowerCamelCase, and so do we.

We want to keep Protobuf-ES simple and focused, and while this change may not impact existing functionality, it does not help with these goals.

The better solution would be to write a function createFrom for your project, which accepts properties with the original Protobuf field name. The reflection API should make this feasible.

@timostamm timostamm closed this Oct 29, 2024
@locene
Copy link
Author

locene commented Oct 30, 2024

OK, no problem.

Could you please revoke my CLA on cla-assistant.io? I signed a bit hastily, and there are some issues regarding my ability to sign the CLA on behalf of my employer. Had I known you weren't planning to merge it, I wouldn't have done so.

Thank you for your understanding.

@timostamm
Copy link
Member

I cannot revoke your CLA. But you can, on https://cla-assistant.io/my-cla

@locene
Copy link
Author

locene commented Oct 30, 2024

No, I can't. The list is empty when I open it, which is why I'm reaching out to you. My understanding is that it requires a maintainer to delete it.

cla-assistant/cla-assistant#907
cla-assistant/cla-assistant#380
cla-assistant/cla-assistant#406

@timostamm
Copy link
Member

Have you tried signing in first on https://cla-assistant.io/ ?

When I do that, I see CLAs I've signed on https://cla-assistant.io/my-cla, and I have a button to revoke:

Screenshot 2024-10-30 at 15 17 48

@locene
Copy link
Author

locene commented Oct 30, 2024

Yes, I am already logged in. Here is my screenshot:

(click to enlarge)

This is the CLA I signed for this repository previously, and you can see that it is still in a signed state:

(click to enlarge)

(click to enlarge)

@timostamm
Copy link
Member

We have looked into it, but we don't see a way to revoke it.

For some context, I don't believe that the state of this PR (or any past ones) will change by revoking a CLA, only future ones. If it would be possible to retroactively revoke a CLA for a contribution, OSS could not safely accept any contributions. But you haven't contributed, and as far as I understand, you have revoked your CLA for future contributions. IANAL, but it looks to me like this is void.

@locene
Copy link
Author

locene commented Oct 30, 2024

Alright, I agree that revoking the CLA won't affect the status of existing PRs, but based on my understanding, the CLA for future contributions is still active, and I think that could lead to potential legal issues.

Thank you very much for your assistance. I will go ahead and open a ticket in the cla-assistant/cla-assistant project.

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 this pull request may close these issues.

3 participants