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.

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 getStore method #58

Merged
merged 19 commits into from
Oct 17, 2023
Merged

feat!: add getStore method #58

merged 19 commits into from
Oct 17, 2023

Conversation

eduardoboucas
Copy link
Member

@eduardoboucas eduardoboucas commented Oct 9, 2023

Updates the API with a breaking change, making a getStore method the entry point into the library.

Before reviewing the actual code, I recommend reviewing the API design itself.

@biilmann your proposed design has been incorporated here.

@eduardoboucas eduardoboucas requested a review from a team as a code owner October 9, 2023 08:49
@netlify
Copy link

netlify bot commented Oct 9, 2023

Deploy Preview for blobs-js ready!

Name Link
🔨 Latest commit 1a951aa
🔍 Latest deploy log https://app.netlify.com/sites/blobs-js/deploys/652eabb0a57d04000832906f
😎 Deploy Preview https://deploy-preview-58--blobs-js.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Oct 9, 2023
@eduardoboucas eduardoboucas marked this pull request as draft October 9, 2023 08:50
@eduardoboucas eduardoboucas marked this pull request as ready for review October 12, 2023 14:16
}
```

### Environment-based configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move this first

src/store.ts Outdated Show resolved Hide resolved
src/store.ts Outdated Show resolved Hide resolved
src/client.ts Outdated Show resolved Hide resolved
src/client.ts Show resolved Hide resolved
src/retry.ts Show resolved Hide resolved
src/store.ts Show resolved Hide resolved
src/store.ts Outdated Show resolved Hide resolved
src/types.ts Outdated
Comment on lines 3 to 6
export enum HTTPMethod {
Delete = 'delete',
Get = 'get',
Put = 'put',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can I bikeshed these to be all caps?

Copy link
Member Author

Choose a reason for hiding this comment

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

The keys or the values?

Copy link
Member Author

Choose a reason for hiding this comment

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

ascorbic
ascorbic previously approved these changes Oct 13, 2023
Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

Excited for this!

lukasholzer
lukasholzer previously approved these changes Oct 17, 2023
Copy link
Contributor

@lukasholzer lukasholzer left a comment

Choose a reason for hiding this comment

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

Awesome! Like the new API 🚀

@@ -13,73 +13,163 @@ You can install `@netlify/blobs` via npm:
npm install @netlify/blobs
```

### Requirements

- Deno 1.30 and above or Node.js 16.0.0 and above
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we still have to support node 14? @eduardoboucas

@@ -74,9 +74,7 @@
"esbuild": "^0.19.0",
"husky": "^8.0.0",
"node-fetch": "^3.3.1",
"p-map": "^6.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Yay 🥇

src/client.ts Outdated
Comment on lines 7 to 11
// The name of the environment variable that holds the context in a Base64,
// JSON-encoded object. If we ever need to change the encoding or the shape
// of this object, we should bump the version and create a new variable, so
// that the client knows how to consume the data and can advise the user to
// update the client if needed.
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: please use JSDoc

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 99a0e4c.

Copy link
Contributor

@lukasholzer lukasholzer left a comment

Choose a reason for hiding this comment

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

LGTM!

@eduardoboucas eduardoboucas merged commit 6507e87 into main Oct 17, 2023
@cbosss
Copy link
Contributor

cbosss commented Oct 17, 2023

@eduardoboucas eduardoboucas deleted the feat/new-api branch October 24, 2023 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants