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

Add safeIdentifier and safeObjectProperty helpers #391

Merged
merged 10 commits into from
Mar 15, 2023

Conversation

dimitropoulos
Copy link
Contributor

It came up in connectrpc/connect-query-es#19 that we need to export the functionality of safely modifying names for identifiers.

This PR adds safeIdentifier and safeObjectProperty helpers which do exactly that. All prior behavior should be the same (except for these functions now being isolated, tested, and exported).

packages/protobuf/src/private/names.ts Outdated Show resolved Hide resolved
packages/protobuf/src/private/names.ts Outdated Show resolved Hide resolved
packages/protobuf/src/private/names.ts Outdated Show resolved Hide resolved
@dimitropoulos
Copy link
Contributor Author

dimitropoulos commented Feb 15, 2023

Now that we found that bug re: prototype inheritance, it looks like it might have been relied on because one of the fixtures fails now. That suggest to me that we'll need to update something in the generator. Would you mind helping me locate what?fixtures fails now. That suggest to me that we'll need to update something in the generator. Would you mind helping me locate what needs to be updated? Is it somewhere in typescript.ts?

Screenshot_20230215_122115

@dimitropoulos dimitropoulos changed the title adds safeIdentifier and safeObjectProperty helpers Adds safeIdentifier and safeObjectProperty helpers Feb 16, 2023
Copy link
Member

@timostamm timostamm left a comment

Choose a reason for hiding this comment

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

Code looks really nice! Two comments though:

packages/protobuf/src/private/names.ts Show resolved Hide resolved
@@ -10,5 +10,5 @@ server would usually do.

| code generator | bundle size | minified | compressed |
|---------------------|------------------------:|-----------------------:|-------------------:|
| protobuf-es | 86,785 b | 36,950 b | 9,642 b |
| protobuf-es | 87,019 b | 36,999 b | 9,692 b |
Copy link
Member

Choose a reason for hiding this comment

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

Can you look into this? We should not increase the bundle size. I know it's tedious for 50 bytes compressed, but we don't make any logical changes here... I'm happy to help looking at the bundle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I definitely need help: I cannot for the life of me determine what's going on. see https://github.com/bufbuild/protobuf-es/tree/dimitri/test-safe-names and note that in the last commit the bundle.js file didn't change, despite the size being reported to be different. I also know it's not from the exports or the test files (by process of elimination).

Copy link
Member

Choose a reason for hiding this comment

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

You didn't see changes because gather() is called twice, once for protobuf-es, once for protobuf-javascript, but you always write to bundle.js, overwriting the protobuf-es bundle output with the protobuf-javascript output, which does not change with this PR.

Copy link
Member

Choose a reason for hiding this comment

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

I merged in main for #411.

Copy link
Member

Choose a reason for hiding this comment

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

Took a quick stab at bundle size in 7c274cf, but only managed to shave off two bytes from the compressed. That doesn't seem worth it, I think the readability of the safe*Property functions and the Set are more important. Reverted in b58ffd7.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I just got that working, yeah, this is the bundle size change

Screenshot_20230315_142335

@timostamm timostamm changed the title Adds safeIdentifier and safeObjectProperty helpers Add safeIdentifier and safeObjectProperty helpers Mar 15, 2023
@dimitropoulos dimitropoulos merged commit 8748217 into main Mar 15, 2023
@dimitropoulos dimitropoulos deleted the dimitri/safe-names branch March 15, 2023 18:30
@smaye81 smaye81 mentioned this pull request Mar 24, 2023
smaye81 added a commit that referenced this pull request Mar 24, 2023
This release contains the following:

## Enhancements
* Add `safeIdentifier` and `safeObjectProperty` helpers by
@dimitropoulos in #391.

**Full Changelog**:
v1.1.1...v1.2.0
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.

None yet

2 participants