Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: support building in CJS and ESM formats #2296

Merged
merged 18 commits into from
Oct 24, 2023
Merged

feat: support building in CJS and ESM formats #2296

merged 18 commits into from
Oct 24, 2023

Conversation

ddelgrosso1
Copy link
Contributor

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

@product-auto-label product-auto-label bot added size: l Pull request size is large. api: storage Issues related to the googleapis/nodejs-storage API. labels Sep 7, 2023
@ddelgrosso1
Copy link
Contributor Author

Need googleapis/gax-nodejs#1459 merged first.

@ddelgrosso1 ddelgrosso1 force-pushed the dual-format branch 6 times, most recently from 7f11b1b to a029333 Compare September 20, 2023 20:08
@ddelgrosso1 ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 20, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 20, 2023
@ddelgrosso1 ddelgrosso1 force-pushed the dual-format branch 3 times, most recently from 7ca5c04 to 5604034 Compare September 21, 2023 18:10
@ddelgrosso1 ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 21, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 21, 2023
@ddelgrosso1 ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 22, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 22, 2023
@ddelgrosso1 ddelgrosso1 marked this pull request as ready for review September 22, 2023 15:13
@ddelgrosso1 ddelgrosso1 requested review from a team as code owners September 22, 2023 15:13
@ddelgrosso1 ddelgrosso1 requested a review from sofisl September 22, 2023 18:35
danielduhh
danielduhh approved these changes Sep 26, 2023
View reviewed changes
.jsdoc.json
"includePattern": "\\.js$"
},
"templates": {
"copyright": "Copyright 2019 Google, LLC.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2023?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just a renamed file / moved file. Should I still update the year?

internal-tooling/performPerformanceTest.ts
appBufferSize: NODE_DEFAULT_HIGHWATER_MARK_BYTES,
crc32cEnabled: false,
md5Enabled: false,
api: 'JSON',
elapsedTimeUs: 0,
cpuTimeUs: -1,
status: 'OK',
chunkSize: argv.range_read_size,
workers: argv.workers,
chunkSize: argv.range_read_size as number,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these casts related to ESM or something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are related to updates in TypeScript not ESM.

src/resumable-upload.ts Outdated
@@ -769,7 +775,9 @@ export class Upload extends Writable {
const headers: GaxiosOptions['headers'] = {
'x-goog-api-client': `${getRuntimeTrackingString()} gccl/${
packageJson.version
} gccl-invocation-id/${this.currentInvocationId.chunk}`,
}-${getModuleFormat()} gccl-invocation-id/${
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker but it would be easier to cipher through this if the header changes was decoupled in a separate PR

sofisl
sofisl reviewed Sep 26, 2023
View reviewed changes
conformance-test/libraryMethods.ts
@@ -21,6 +21,7 @@ import {
deleteTestFile,
} from './testBenchUtil';
import * as uuid from 'uuid';
import {getDirName} from '../src/util.js';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB, you could save dirName = getDirName(), that way you improve performance vs. memory (seems like you call it in multiple spots)

@sofisl
Copy link
Contributor

sofisl commented Sep 26, 2023

Exciting stuff!

@ddelgrosso1 ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 27, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 27, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 29, 2023
@ddelgrosso1 ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 6, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 6, 2023
@ddelgrosso1 ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 20, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 20, 2023
@ddelgrosso1 ddelgrosso1 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 20, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 20, 2023
@ddelgrosso1 ddelgrosso1 merged commit c848076 into googleapis:main Oct 24, 2023
@ddelgrosso1 ddelgrosso1 deleted the dual-format branch October 24, 2023 19:36
@release-please release-please bot mentioned this pull request Oct 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielbankhead danielbankhead danielbankhead approved these changes

@danielduhh danielduhh danielduhh approved these changes

@sofisl sofisl sofisl approved these changes

Assignees
No one assigned
Labels
api: storage Issues related to the googleapis/nodejs-storage API. size: l Pull request size is large.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ddelgrosso1 @sofisl @danielduhh @danielbankhead @yoshi-kokoro