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: Reduce code size by using nullish coalescing operator in fromPartial #376

Merged
merged 8 commits into from
Nov 2, 2021

Conversation

webmaster128
Copy link
Collaborator

@webmaster128 webmaster128 commented Oct 26, 2021

Closes #371

Builds on #375 (merge that first, then rebase)

} else {
message.numberField = 0;
{
message.numberField = object.numberField ?? 0;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This block is not needed in the output code. However, I can't get it to work without it.

src/main.ts Outdated
isValueType(ctx, field)
) {
// An optimized case of the else below that works when `readSnippet` returns the plain input
chunks.push(code`{`); // Without this extra scope the code generation breaks 🤷. We don't really need it.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Any clue why thi line is needed? Without it the code generation breaks compeltely.

Copy link
Owner

Choose a reason for hiding this comment

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

@webmaster128 wow, that was weird. I pushed a commit to this PR that re-fiddled with the braces in fromPartial. I'm not really sure how it was working before :-) but I think it's straightened out now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, very nice, thanks. I thought there is some magic in the code generator such that braces are not supposed to be closed manually. Now it's clearer.

webmaster128 added a commit to confio/cosmjs-types that referenced this pull request Nov 1, 2021
@webmaster128 webmaster128 marked this pull request as ready for review November 1, 2021 21:10
@webmaster128
Copy link
Collaborator Author

Alright, I'm happy with this now. In https://github.com/confio/cosmjs-types/pull/11/files you see what this change means for the code generated larger projects.

Rebased onto current main branch.

How do you want this to be released? Is this a feature or a fix?

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.

@webmaster128 looks great! Much cleaner! Yeah, releasing as a feature sgtm.

@webmaster128
Copy link
Collaborator Author

Thanks :)

@webmaster128 webmaster128 merged commit 19d2ded into stephenh:main Nov 2, 2021
@webmaster128 webmaster128 deleted the organize-intitializers branch November 2, 2021 07:20
stephenh pushed a commit that referenced this pull request Nov 2, 2021
# [1.84.0](v1.83.3...v1.84.0) (2021-11-02)

### Features

* Reduce code size by using nullish coalescing operator in fromPartial ([#376](#376)) ([19d2ded](19d2ded))
@stephenh
Copy link
Owner

stephenh commented Nov 2, 2021

🎉 This PR is included in version 1.84.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.

Use nullish coalescing operator (??) for assigning binaries
2 participants