fix: usePrototypeForDefaults with repeated fix #499
Conversation
stephenh
force-pushed
the
main
branch
4 times, most recently
from
cdf2835
to
7017d4c
Compare
| @@ -1004,42 +1011,49 @@ export const SimpleWithMap = { | |||
| const tag = reader.uint32(); | |||
| switch (tag >>> 3) { | |||
| case 1: | |||
| message.entitiesById ||= []; | |||
There was a problem hiding this comment.
Shouldn't all the properties under this SimpleWithMap be instantiated with an empty object, i.e. {} instead of array?
| @@ -736,7 +736,7 @@ function generateBaseInstanceFactory( | |||
| : isMapType(ctx, messageDesc, field) | |||
| ? '{}' | |||
There was a problem hiding this comment.
Do we need to account the map case as well?
|
Hi @pbhuynh , just taking a look--sorry @alexeyten , you had some great work here, and I must not have had time to review it/think about it when you first submitted the PR. In terms of approach, I think if I was personally using That said, I think what the current PR is doing, just making the repeated fields undefined, is probably more straight-forward/an easier path to getting merged. Just in general @pbhuynh , I think next steps would be 1) getting the PR rebased and 2) have a clear "failed before, passing now" test/reproduction in the integration tests. |
Just an idea how to fix #498.
Declare repeated fields as “possibly undefined” and create them on demand. So “hazzle check” would still work, but callers will have to create array manually.