feat!: better typing for metadata #2234

ddelgrosso1 · 2023-07-12T21:27:36Z

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 #1817 🦕

frankyn

Had a question on if this is a breaking change; I think so, and trying to understand how Sameena's concerns were concluded. I'm still reviewing the PR so just an initial comment.

frankyn · 2023-07-12T23:25:21Z

src/acl.ts

+    projectNumber?: string;
+    team?: 'editors' | 'owners' | 'viewers';
+  };
+  role?: 'OWNER' | 'READER' | 'WRITER' | 'FULL_CONTROL';


is this strict typing in that if new fields are added in the API the node.js client will need to be updated as well?

https://github.com/googleapis/nodejs-storage/pull/2234/files#diff-747d768492a54441f1682714c15429c572d74c031145c71672dec819ff94fbbbR109 this would act as a catch all for anything new so that it flows through without us having to make a change. However, we would obviously want to add anything new to the appropriate metadata interface to continue to support up to date typings.

I like the unknown too - encourages customers to validate it

danielbankhead

This is a great change - customers are gonna love it (including myself).

package.json

danielbankhead · 2023-07-13T19:22:54Z

src/acl.ts

+    projectNumber?: string;
+    team?: 'editors' | 'owners' | 'viewers';
+  };
+  role?: 'OWNER' | 'READER' | 'WRITER' | 'FULL_CONTROL';


I like the unknown too - encourages customers to validate it

frankyn

I have a few comments; the types looked appropriate from my review and more wondering about the use of unknown

frankyn · 2023-07-17T20:02:42Z

src/bucket.ts

 export interface GetFilesCallback {
  (
    err: Error | null,
    files?: File[],
    nextQuery?: {},
-    apiResponse?: Metadata
+    apiResponse?: unknown


is there more guidance on when to use unknown?

Here is some docs.

The reason I had to go with unknown is sort of twofold, first previously Metadata was defined as any and was being used to represent ACTUAL metadata as well as a raw API response. Ideally we would have a well defined type for an API response but seeing as we are still using 2 HTTP libraries that is a bit more difficult currently.

frankyn · 2023-07-17T20:03:49Z

src/bucket.ts

 }

 export interface GetLabelsOptions {
  userProject?: string;
 }

-export type GetLabelsResponse = [Metadata];
+export type GetLabelsResponse = [unknown];


Why are API responses being labelled unknown?

See my comments above, the short of it is that previously API responses were any and due to the slightly different shapes of responses between Gaxios and teeny-request I chose to go with unknown until we can more clearly type API responses.

frankyn · 2023-07-17T20:04:35Z

src/bucket.ts

+  acl?: AclMetadata[] | null;
+  autoclass?: {
+    enabled?: boolean;
+    toggleTime?: string;


should this be Date as well?

frankyn · 2023-07-22T00:42:50Z

src/bucket.ts

+  acl?: AclMetadata[] | null;
+  autoclass?: {
+    enabled?: boolean;
+    toggleTime?: string;


)

* test: cleanup kms tests to avoid setting incorrect keys (#2213) * chore(deps): update dependency c8 to v8 (#2221) * feat!: better typing for metadata * more metadata typing * fix merge problems * remove extend * fix merge conflicts --------- Co-authored-by: Mend Renovate <[email protected]>

* refactor!: Remove `extend` and Treat Provided Options as Mutable (#2204) * refactor: Remove `extend` This is the prelimary, `src`-only commit. * fix: object merging * chore: remove log * chore: minor clean-up * chore: more clean-up * refactor: remove `extend` * perf!: treat provided options as mutable * feat!: stronger typing for lifecycle rules (#2215) * fix!: Do not return responsebody in delete, only raw response (#2236) * fix!: Remove extraneous validation in favor of server errors (#2237) * fix!: Remove extraneous validation in favor of server errors * remove test * refactor!: mark bucket.setLabels and associated interfaces as deprecated (#2214) * refactor!: mark bucket.setLabels and associated interfaces as deprecated * update addBucketLabel to use setMetadata * mark getLabels / deleteLabels deprecated, update samples * feat!: disable source maps for smaller bundle size (#2240) * feat!: better typing for metadata (#2234) * test: cleanup kms tests to avoid setting incorrect keys (#2213) * chore(deps): update dependency c8 to v8 (#2221) * feat!: better typing for metadata * more metadata typing * fix merge problems * remove extend * fix merge conflicts --------- Co-authored-by: Mend Renovate <[email protected]> * chore!: make node 14 the minimum version (#2230) * chore!: make node 14 the minimum version * move conformance test to node 14 * package upgrades, fix unit test compilation * update google owned deps * fix tests * remove node 12 kokoro folders * merge and fix issues * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * export new metadata types from index * fix samples tests * fix system test --------- Co-authored-by: Daniel Bankhead <[email protected]> Co-authored-by: Mend Renovate <[email protected]> Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>

ddelgrosso1 requested review from a team as code owners July 12, 2023 21:27

product-auto-label bot added size: xl Pull request size is extra large. api: storage Issues related to the googleapis/nodejs-storage API. labels Jul 12, 2023

ddelgrosso1 force-pushed the metadata-typing branch from aa6809e to b5a36a0 Compare July 12, 2023 21:40

ddelgrosso1 requested review from frankyn and danielbankhead July 12, 2023 21:42

frankyn reviewed Jul 12, 2023

View reviewed changes

danielbankhead approved these changes Jul 13, 2023

View reviewed changes

frankyn reviewed Jul 20, 2023

View reviewed changes

frankyn approved these changes Jul 22, 2023

View reviewed changes

src/bucket.ts

acl?: AclMetadata[] | null;

autoclass?: {

enabled?: boolean;

toggleTime?: string;

Copy link

Member

frankyn Jul 22, 2023

Choose a reason for hiding this comment

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

nvm!

ddelgrosso1 and others added 7 commits July 24, 2023 13:33

test: cleanup kms tests to avoid setting incorrect keys (googleapis#2213

a89df85

)

chore(deps): update dependency c8 to v8 (googleapis#2221)

6d465da

feat!: better typing for metadata

f244eed

more metadata typing

ddcc98b

fix merge problems

32c60ff

remove extend

d48a822

fix merge conflicts

46bef73

ddelgrosso1 force-pushed the metadata-typing branch from acae384 to 46bef73 Compare July 24, 2023 13:44

ddelgrosso1 merged commit ee8354e into googleapis:storage-seven Jul 24, 2023

ddelgrosso1 deleted the metadata-typing branch July 24, 2023 13:53

danielbankhead mentioned this pull request Jul 25, 2023

Add typing for File.metadata #1817

Closed

runeh mentioned this pull request Aug 25, 2023

Undocumented breaking change in v7 #2285

Closed

anantakrishna mentioned this pull request Jan 10, 2024

Apparently wrong type of CopyOptions.metadata #2389

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: better typing for metadata #2234

feat!: better typing for metadata #2234

ddelgrosso1 commented Jul 12, 2023

frankyn left a comment

frankyn Jul 12, 2023

ddelgrosso1 Jul 13, 2023

danielbankhead Jul 13, 2023

danielbankhead left a comment

danielbankhead Jul 13, 2023

frankyn left a comment

frankyn Jul 17, 2023

ddelgrosso1 Jul 21, 2023

frankyn Jul 17, 2023

ddelgrosso1 Jul 21, 2023

frankyn Jul 17, 2023

frankyn Jul 22, 2023

frankyn Jul 22, 2023

feat!: better typing for metadata #2234

feat!: better typing for metadata #2234

Conversation

ddelgrosso1 commented Jul 12, 2023

frankyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielbankhead left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment