-
Notifications
You must be signed in to change notification settings - Fork 87
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: add ESM tools in gax #1459
feat: add ESM tools in gax #1459
Conversation
Want to get some eyes on this before I dive deeper into other transformations. Will this be acceptable for our needs? |
222cbb6
to
acb7493
Compare
Thanks @ddelgrosso1, this is looking awesome! A few questions/things that I think this still needs to do:
|
@sofisl regarding your points above 1. I think this can be handled by specifying the appropriate babel cli arguments and won't require custom plugin code. 2. Changing the variable should be easy but are you saying we need to be aware when a tsconfig is in use? If so, that might prove more challenging. 3. Let me look into this one further. |
@ddelgrosso1 awesome! For #2, ideally we'd know whether we compiled using tsconfig.esm, and that would determine whether that variable was true or not. |
74998cd
to
5a2916e
Compare
43ee800
to
112b3d9
Compare
assert(ts.toString().includes('import Long = require')); | ||
assert(!ts.toString().includes('import * as 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.
Just curious - would import Long from
work here?
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.
Good call out! I think it would, but I'm just looking to preserve functionality here, since I'm not making any changes to the .ts file.
tools/src/compileProtos.ts
Outdated
@@ -163,6 +163,13 @@ function updateDtsTypes(dts: string, enums: Set<string>): string { | |||
} | |||
|
|||
function fixJsFile(js: string): string { | |||
// 1. fix protobufjs import: we don't want the libraries to |
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.
super nit but let's either make this "0." or increment the numbers below :)
tools/src/compileProtos.ts
Outdated
// depend on protobufjs, so we re-export it from google-gax | ||
js = js.replace( | ||
'import * as $protobuf from "protobufjs/minimal"', | ||
'import { protobufMinimal as $protobuf} from "google-gax/build/src/protobuf.js"' |
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.
nit: formatting, remove space after {
This PR does the following:
path.dirname(fileURLToPath(import.meta)
to__dirname
, another to turnisEsm
to false, and lastly a final one to turn proxyquire into esmock.protos.js
andprotos.cjs
in es6 and amd, respectively)