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(bindings/nodejs): Make PresignedRequest serializable #1797

Merged
merged 1 commit into from
Mar 29, 2023

Conversation

suyanhanx
Copy link
Member

@suyanhanx suyanhanx commented Mar 29, 2023

As the properties of PresignedRequest are all in getter or function form, they cannot be serialized on the JS side.

Change PresignedRequest to type object to be serializable.

bindings/nodejs/index.d.ts Outdated Show resolved Hide resolved
@Xuanwo
Copy link
Member

Xuanwo commented Mar 29, 2023

they cannot be serialized on the JS side.

Does 'serialize' mean that users want to serialized the returning request directly?

This PR provides a helper function for processing.

No, it's not the job of bindings. We need to offer native-alike functions for the language.

@suyanhanx
Copy link
Member Author

Does 'serialize' mean that users want to serialized the returning request directly?

Yes.

We need to offer native-alike functions for the language.

Getters or methods as part of a class cannot be serialized.
I don't think providing such a method would compromise the native-alike experience.

@Xuanwo
Copy link
Member

Xuanwo commented Mar 29, 2023

Getters or methods as part of a class cannot be serialized.

If we intend to enable users to serialize the struct that we have returned, it implies a commitment on our part not to alter the layout of the struct. Therefore, returning an Object is the only viable option. Introducing a new API such as toJSON will not address this issue.

@Xuanwo
Copy link
Member

Xuanwo commented Mar 29, 2023

Please update the PR's title too😜, mostly LGTM!

@suyanhanx suyanhanx changed the title feat(bindings/nodejs): Support PresignedRequest toJSON feat(bindings/nodejs): Change PresignedRequest to type object to be serializable Mar 29, 2023
@Xuanwo Xuanwo changed the title feat(bindings/nodejs): Change PresignedRequest to type object to be serializable feat(bindings/nodejs): Make PresignedRequest serializable Mar 29, 2023
@Xuanwo
Copy link
Member

Xuanwo commented Mar 29, 2023

Thanks a lot!

@Xuanwo Xuanwo merged commit c7d975e into apache:main Mar 29, 2023
@suyanhanx suyanhanx deleted the presign-r-se branch March 29, 2023 12:56
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.

2 participants