-
Notifications
You must be signed in to change notification settings - Fork 104
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
fix: correct Long types import #358
Conversation
ergh no, the generator removes the manual edit I made 😅 Does need the dependency in package.json, though |
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.
Thank you so much for submitting a fix! And sorry for the 🤦♂️ You were super close.
@@ -1,5 +1,5 @@ | |||
import * as $protobuf from "protobufjs"; | |||
import * as long from "long"; | |||
import * as Long from "long"; |
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.
Yarrr, nice find and sorry for the mixup. What you really wanna do here is update this line in package.json:
"proto:datastore": "mkdir -p proto && pbjs -t static-module -w commonjs -p node_modules/google-proto-files google/datastore/v1/datastore.proto | pbts -i long -o proto/datastore.d.ts -",
Make that:
"proto:datastore": "mkdir -p proto && pbjs -t static-module -w commonjs -p node_modules/google-proto-files google/datastore/v1/datastore.proto | pbts -i Long -o proto/datastore.d.ts -",
That way the fix doesn't get roughed up by the generator.
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.
Then it's import Long from 'Long'
which I guess will go boom on linux?
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.
Nah, it'll probably be fine
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.
This very much will blow up on Linux -- import filenames are case sensitive there. Applying the suggestion results in:
> @google-cloud/datastore@3.1.2 compile /home/mgl/3psrc/nodejs-datastore
> tsc -p . && cp -r src/v1 build/src && cp -r proto* build && cp test/*.js build/test
proto/datastore.d.ts:2:23 - error TS2307: Cannot find module 'Long'.
2 import * as Long from "Long";
~~~~~~
Found 2 errors.
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.
Is this what we want? protobufjs/protobuf.js#1166
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.
Yes lol. That would fix all the things.
Codecov Report
@@ Coverage Diff @@
## master #358 +/- ##
=======================================
Coverage 98.05% 98.05%
=======================================
Files 5 5
Lines 617 617
Branches 145 145
=======================================
Hits 605 605
Partials 12 12 Continue to review full report at Codecov.
|
@JustinBeckwith in some of the other clients we are just editing the generated file until a better solution presents itself. Assuming we're cool doing that here as well, I think this PR is good to go unless you have any objections |
SGTM. Let's just be careful about not blasting these away :) |
@JustinBeckwith I see you became a contributor to Protobuf.js 😮 any chance we'll see a release soon with the Long fix? |
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.
Going to merge this, but until the needed patch for protobufjs is released we should be mindful not to overwrite this change by re-generating types.
Fixes #345
Ref #352