-
Notifications
You must be signed in to change notification settings - Fork 88
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
chore!: publish gax-nodejs in dual format (CJS and ESM) #1679
base: main
Are you sure you want to change the base?
Conversation
… into migrateToEsm
… into migrateToEsm
Warning: This pull request is touching the following templated files:
|
"main": "./build/cjs/src/index.cjs", | ||
"type": "module", | ||
"types": "./build/cjs/src/index.d.ts", |
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.
A quick note: the main should be .js
/.mjs
when type: esm
- some, namely older, build tools won't work well with the conflict. Alternatively, it may be easier to use type: commonjs
as the default
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.
@danielbankhead, do you have an example of where this won't work (that we claim to support)? This would be an awesome test to add in if so! Reason I'm asking is because this design is what we're doing for our other repos, specifically all our client libraries (see storage), so it's worth confirming if this actually won't work.
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.
Webpack 4- and earlier versions of eslint will fail here.
Additionally, in the storage example the main
ends with .js
rather than .cjs
.
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 think of it not whether we claim to support it or not, but rather are we following guidance and standards appropriately. If not, we can expect some customers to run into friction.
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.
Hm, isn't storage still using a cjs file (seems like the js file is in the cjs module?) Also we are running tests using webpack 4 for the browser and they are WAI!
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.
Regardless, I don't feel strongly about this since we're testing gax in our system tests, and I think the exports field have precedence over main anyways. Running tests to make sure everything passes.
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.
Update, seems like this actually breaks browser tests: https://btx.cloud.google.com/invocations/37842334-df7a-4bd8-8aef-a97b319b0cd7/targets/cloud-devrel%2Fclient-libraries%2Fnodejs%2Fpresubmit%2Fgoogleapis%2Fgax-nodejs%2Fnode18%2Fbrowser-test/log
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.
FWIW, this was my attempt when adding back in a browser
field to default to cjs and then use esm as the entrypoint: https://btx.cloud.google.com/invocations/feb590d8-03ab-40b6-bf59-fcd445db5f47/targets/cloud-devrel%2Fclient-libraries%2Fnodejs%2Fpresubmit%2Fgoogleapis%2Fgax-nodejs%2Fnode18%2Fbrowser-test/log
@@ -0,0 +1,50 @@ | |||
const fs = require('fs'); |
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.
Why not add a package.json with type: commonjs
? According to spec:
Additionally reading/examples for this pattern:
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 is for changing the extension in proxyquire calls, so it wouldn't be affected by a package.json, although I see your larger point that we wouldn't even have to do that if we didn't rename the files at all. It seems slightly cleaner/safer to me to have the .cjs and .js distinguishing the files, easier to see at a glance what's going on, but that's just my perspective!
… and defaulting to esm"
This is a massive PR... If I could make it any smaller, believe me, I would!
Changes for Node 18:
Some ESM syntax we're using requires Node 18 (for example, using
with
assertions to import JSON). So, this PR also includes the bare minimum needed to migrate gax to Node 18:Changes for ESM:
Publishing Nodejs libraries in ESM and CJS
)import {CallSettings} from './gax'
-->import {CallSettings} from './gax.js'
with {type: 'json'};
extension (this is a weak spot for ESM; as of Node 18 you can import usingwith
assertion)const dirname = path.dirname(fileURLToPath(import.meta.url));
(seePublishing Nodejs libraries in ESM and CJS
) - we made a custom babel plugin to also transpile this to __dirname for cjs.esm/src/...
. References to these files have added a directory level.Tests
Tools
\nconst $protobuf = protobuf.default
to all of our protos; this will happen when we compileProtos, which is why we'll need to rerun before submitting.Migration Guide
this._gaxModule = opts.fallback ? gaxInstance.fallback : gaxInstance;
in theclient.ts
; the reason is that client libraries, in their CJS incarnation, optionally asynchronously load gax in the line before. However since it is not imported at the top of the file, ESM thinks it's not being loaded, when in fact in ESM it is not conditionally required (it is just asynchronously loaded due to being a regular ESM import). TLDR: TS doesn't realize gax will always be loaded, so we need to ignore the warning. We must add this to our generated client library code.["allowSyntheticDefaultImports": true](https://www.typescriptlang.org/tsconfig/#allowSyntheticDefaultImports)
in our tsconfigs to appropriately load gax.compileProtos
before running tests in system tests. Since we are slightly modifying how we compileProtos, we'll need to recompile before running tests.Package.json changes
main
andtypes
default to CJS, while the module is actually overall type asmodule
(i.e., esm). @danielbankhead, do you have an example of where this won't work (that we claim to support)? This would be an awesome test to add in if so! Reason I'm asking is because this design is what we're doing for our other repos, specifically all our client libraries (see storage), so it's worth confirming if this actually won't work.-w es6
command, which outputs the JS to ES6 instead of UMD.esm
directory levelPublishing Nodejs libraries in ESM and CJS
: basically takes the TS output and runs some custom babel plugins, as well as copies over files, and puts files into the cjs directory.TODO for after: