feat: added nestJsTimestampTypeWrapper #567

PhilipMantrov · 2022-05-17T12:53:29Z

related issue #69

stephenh · 2022-05-18T14:24:24Z

@PhilipMantrov nice! Would be amazing to get this fixed.

I think as-is, the added code isn't going to actually get output? Or will it? Usually code within that makeUtils function is used for things we want conditionally generated, so I just assumed that would be the case here.

Can we nudge one of the existing integration-test examples around to show this code in it's output, and maybe even have a unit test that shows it works? Fwiw I'm fine w/o the unit test if it means just getting this landed, given a lot of users have been asking for it.

Thanks!

PhilipMantrov · 2022-05-19T14:59:03Z

Added integration test and change
const wrappers = imp('wrappers@protobufjs/minimal');
to
const wrappers = imp('wrapper@protobufjs');
because protobuf.js has custom wrapper only for Any (wrappers.js, and issue), and if we dont have Any in our proto, wrappers will be undefined

PhilipMantrov · 2022-05-19T15:02:11Z

And I commit changes in genfiles, if it isn't needed just revert this commit.

merge original master into fork

PhilipMantrov · 2022-05-25T18:26:56Z

@stephenh can we merge this pr asap? Or just tell me what we need this for accepting this pr, thx. :)

stephenh · 2022-05-26T01:52:06Z

integration/angular/simple-message.ts

@@ -1,6 +1,6 @@
 /* eslint-disable */
+import { util, configure, Writer, Reader } from 'protobufjs/minimal';


@PhilipMantrov do you know why all of these other files changed?

@stephenh newest version of protobufjs has deep types, so compiler dont need abstraction as _m0 anymore.
In package.json we have '^', so we install newest version 6.11.3 (on 6.8.8 i see old behavior and compiler use _m0 for abstraction).

Huh! I'd love to get rid of this, but I'm not following why this would change, b/c:

Isn't yarn.lock pinned to 6.8.8?

What controls the _m0 output is this line:

https://github.com/stephenh/ts-proto/blob/main/src/options.ts#L183

I.e. the * as _m0 is generated by ts-poet because we explicitly tell it to, and I'm like 99% sure nothing in node_modules/protobuf/... would affect ts-poet's behavior as to whether it does/does not use the _m0 style import...

So, dunno, I'm still not following how this changed?

@PhilipMantrov I checked out this PR locally, and when I run yarn bin2ts, AFAICT it puts all of the _m0 back... I'm wondering if maybe when you ran codegen, you weren't on the latest main and/or had an outdated version of ts-poet installed?

Fwiw I'd be really happy to bump protobufjs and clean up these imports, but seems like something to do in a separate PR.

Can you try a new yarn bin2ts and see what happens? Thanks!

@PhilipMantrov I got your ping about asking for a re-review, see this comment ^.

If we just can't figure this out, I can always create a new PR with your core code changes, and w/o all of the _m0 changes, and get that merged.

protobufjs now fixed on 6.8.8

I'd be really happy to bump protobufjs and clean up these imports, but seems like something to do in a separate PR

I create another PR for this soon.

stephenh · 2022-05-26T01:57:12Z

@PhilipMantrov thanks for adding the test! I'm good to merge this once the build is passing.

PhilipMantrov · 2022-05-26T10:42:17Z

@PhilipMantrov thanks for adding the test! I'm good to merge this once the build is passing.

@stephenh you need to disable 'Diff check' for once because protobufjs was updated.

n0isy · 2022-07-14T14:37:15Z

Any updates?

merge master repo

PhilipMantrov · 2022-07-20T09:38:45Z

Something strange with tests, working well in local env, but failed in CI.
@stephenh can you take a look?

gusinacio · 2022-07-26T20:14:48Z

I tried to run the assignment of ".google.protobuf.Timestamp" to wrappers with protobufjs 7.0.0 and it isn't working, I had to downgrade to 6.11.3.

It's worth to check if it's running in version 7 and what has changed.

PhilipMantrov · 2022-08-30T10:22:49Z

@flametuner yea, its not working on protobufjs: 7.0.0 and above.
ts-proto was pinned protobufjs to 6.8.8 for some reason, and for protobufjs 7.0.0 need some work in ts-proto.

Just use for now

Add in main.ts:

// ToDo: Remove when https://github.com/stephenh/ts-proto/pull/567 has merged
registerWrapperTypes();

function:

import { wrappers } from 'protobufjs';

export function registerWrapperTypes() {
  wrappers['.google.protobuf.Timestamp'] = {
    fromObject(value) {
      return {
        seconds: value.getTime() / 1000,
        nanos: (value.getTime() % 1000) * 1e6,
      };
    },
    toObject(message: { seconds: number; nanos: number }) {
      return new Date(message.seconds * 1000 + message.nanos / 1e6);
    },
  } as any;
}

TakeOver · 2022-10-11T17:55:38Z

Whatsup guys when this will be merged? I need this feature ASAP

TakeOver · 2022-10-13T12:33:59Z

Is this repo alive? Seems like dead perhaps I should fork it

stephenh · 2022-10-13T13:45:50Z

@TakeOver happy to have you fork the repo; if you want to contribute here I'm happy to merge PRs as well.

For this one, it took a few iterations and I didn't realize it'd been rewritten quite a bit and the build was green. Thanks @PhilipMantrov !

Fwiw technically it is green-ish b/c as @PhilipMantrov mentioned the tests were failing in CI so they're currently being skipped:

  xit('should findOneHero', async () => {
    const hero = await heroService.findOneHero({ id: 1 }).toPromise();
    expect(hero).toEqual({ id: 1, name: 'Stephenh', birthDate: new Date("2000/01/01") });
  });

I didn't have time to debug those, so just fyi if you try using this feature, and it doesn't work, it would be interesting see if the failures are for the same issue as the xit-d tests. Or maybe it really is CI specific.

# [1.128.0](v1.127.0...v1.128.0) (2022-10-13) ### Features * added nestJsTimestampTypeWrapper ([#567](#567)) ([59d451e](59d451e))

stephenh · 2022-10-13T14:03:39Z

🎉 This PR is included in version 1.128.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

TannerPerrien · 2022-12-12T21:08:42Z

@TakeOver and @stephenh - Thanks for this contribution!

When generating output with --ts_proto_opt=useDate=date, I see the resulting protobufjs wrappers/fromObject/toObject. However, my nestjs application does not end up invoking this wrapper and my resulting Timestamp output is

{
  "seconds": "0",
  "nanos": 0
}

I have carefully reviewed the nestjs-simple-usedate integration test.

UPDATE: Leaving the above comment for reference to others.

I re-read this thread and saw the comment about this not working in protobufjs: 7.0.0. It turns out @grpc/proto-loader 0.7.1 and above uses protobufjs: 7.x.x. I downgraded @grpc/proto-loader to 0.6.13 and everything is now working.

stephenh · 2022-12-15T02:27:16Z

Ah nice! Thanks for both the self-diagnosis and also reporting back the "pin to @grpc/proto-loader 0.6.13" fix @TannerPerrien !

feat: added nestJsTimestampTypeWrapper

5d14d97

PhilipMantrov force-pushed the main branch from 3ecc0a8 to 5d14d97 Compare May 17, 2022 12:57

PhilipMantrov added 2 commits May 19, 2022 17:55

fix: wrappers import

75b8c90

feat(integration): added nestjs-simple-usedate

9aca747

fix: genfiles changes

e0568ee

Merge pull request #1 from stephenh/main

c3d4823

merge original master into fork

stephenh reviewed May 26, 2022

View reviewed changes

stephenh force-pushed the main branch 4 times, most recently from cdf2835 to 7017d4c Compare May 28, 2022 17:39

PhilipMantrov requested a review from stephenh June 2, 2022 10:50

PhilipMantrov added 2 commits July 12, 2022 13:23

revert: protobufjs now fixed on 6.8.8

e0b85cd

Merge branch 'main' into main

d5bd5fa

Merge pull request #2 from stephenh/main

4081e02

merge master repo

PhilipMantrov force-pushed the main branch from 83ba587 to 023f71d Compare July 19, 2022 18:23

update integrations

9a687fe

PhilipMantrov force-pushed the main branch from 023f71d to 9a687fe Compare July 19, 2022 18:44

improve codegen check for ProtobufTimestampWrapper

6e5f03d

PhilipMantrov force-pushed the main branch from 5a2666a to 6e5f03d Compare July 19, 2022 22:59

PhilipMantrov added 2 commits July 20, 2022 11:49

fix lint and rename test

d32ba3c

add additional test

5950659

excluded 2 tests thats failed in CI

227a6d0

stephenh merged commit 59d451e into stephenh:main Oct 13, 2022

stephenh pushed a commit that referenced this pull request Oct 13, 2022

chore(release): 1.128.0 [skip ci]

89af942

# [1.128.0](v1.127.0...v1.128.0) (2022-10-13) ### Features * added nestJsTimestampTypeWrapper ([#567](#567)) ([59d451e](59d451e))

stephenh added the released label Oct 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added nestJsTimestampTypeWrapper #567

feat: added nestJsTimestampTypeWrapper #567

PhilipMantrov commented May 17, 2022

stephenh commented May 18, 2022

PhilipMantrov commented May 19, 2022 •

edited

Loading

PhilipMantrov commented May 19, 2022 •

edited

Loading

PhilipMantrov commented May 25, 2022

stephenh May 26, 2022 •

edited

Loading

PhilipMantrov May 26, 2022 •

edited

Loading

stephenh May 26, 2022

stephenh May 27, 2022

stephenh Jun 2, 2022

PhilipMantrov Jul 12, 2022

PhilipMantrov Jul 19, 2022 •

edited

Loading

stephenh commented May 26, 2022

PhilipMantrov commented May 26, 2022 •

edited

Loading

n0isy commented Jul 14, 2022

PhilipMantrov commented Jul 20, 2022

gusinacio commented Jul 26, 2022

PhilipMantrov commented Aug 30, 2022 •

edited

Loading

TakeOver commented Oct 11, 2022

TakeOver commented Oct 13, 2022

stephenh commented Oct 13, 2022

stephenh commented Oct 13, 2022

TannerPerrien commented Dec 12, 2022 •

edited

Loading

stephenh commented Dec 15, 2022

		@@ -1,6 +1,6 @@
		/* eslint-disable */
		import { util, configure, Writer, Reader } from 'protobufjs/minimal';

feat: added nestJsTimestampTypeWrapper #567

feat: added nestJsTimestampTypeWrapper #567

Conversation

PhilipMantrov commented May 17, 2022

stephenh commented May 18, 2022

PhilipMantrov commented May 19, 2022 • edited Loading

PhilipMantrov commented May 19, 2022 • edited Loading

PhilipMantrov commented May 25, 2022

stephenh May 26, 2022 • edited Loading

Choose a reason for hiding this comment

PhilipMantrov May 26, 2022 • edited Loading

Choose a reason for hiding this comment

stephenh May 26, 2022

Choose a reason for hiding this comment

stephenh May 27, 2022

Choose a reason for hiding this comment

stephenh Jun 2, 2022

Choose a reason for hiding this comment

PhilipMantrov Jul 12, 2022

Choose a reason for hiding this comment

PhilipMantrov Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

stephenh commented May 26, 2022

PhilipMantrov commented May 26, 2022 • edited Loading

n0isy commented Jul 14, 2022

PhilipMantrov commented Jul 20, 2022

gusinacio commented Jul 26, 2022

PhilipMantrov commented Aug 30, 2022 • edited Loading

TakeOver commented Oct 11, 2022

TakeOver commented Oct 13, 2022

stephenh commented Oct 13, 2022

stephenh commented Oct 13, 2022

TannerPerrien commented Dec 12, 2022 • edited Loading

stephenh commented Dec 15, 2022

PhilipMantrov commented May 19, 2022 •

edited

Loading

PhilipMantrov commented May 19, 2022 •

edited

Loading

stephenh May 26, 2022 •

edited

Loading

PhilipMantrov May 26, 2022 •

edited

Loading

PhilipMantrov Jul 19, 2022 •

edited

Loading

PhilipMantrov commented May 26, 2022 •

edited

Loading

PhilipMantrov commented Aug 30, 2022 •

edited

Loading

TannerPerrien commented Dec 12, 2022 •

edited

Loading