-
Notifications
You must be signed in to change notification settings - Fork 337
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: added nestJsTimestampTypeWrapper #567
Conversation
@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 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! |
Added integration test and change |
And I commit changes in genfiles, if it isn't needed just revert this commit. |
merge original master into fork
@stephenh can we merge this pr asap? Or just tell me what we need this for accepting this pr, thx. :) |
@@ -1,6 +1,6 @@ | |||
/* eslint-disable */ | |||
import { util, configure, Writer, Reader } from 'protobufjs/minimal'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PhilipMantrov do you know why all of these other files changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
protobufjs now fixed on 6.8.8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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. |
cdf2835
to
7017d4c
Compare
Any updates? |
merge master repo
Something strange with tests, working well in local env, but failed in CI. |
I tried to run the assignment of ".google.protobuf.Timestamp" to It's worth to check if it's running in version 7 and what has changed. |
@flametuner yea, its not working on Just use for now Add in main.ts:
function:
|
Whatsup guys when this will be merged? I need this feature ASAP |
Is this repo alive? Seems like dead perhaps I should fork it |
@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:
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 |
# [1.128.0](v1.127.0...v1.128.0) (2022-10-13) ### Features * added nestJsTimestampTypeWrapper ([#567](#567)) ([59d451e](59d451e))
🎉 This PR is included in version 1.128.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
@TakeOver and @stephenh - Thanks for this contribution! When generating output with {
"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 |
Ah nice! Thanks for both the self-diagnosis and also reporting back the "pin to |
related issue #69