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

chore(deps): support @netlify/blobs v8 #486

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

serhalp
Copy link

@serhalp serhalp commented Sep 15, 2024

The peer dependency only specifies support for v6 and v7, but v8 works just fine.

Closes #485.

This adds v8 to the peer dependencies, bumps the dev dep to v8, updates the tests (there's a new requirement that's specific to unusual use cases like unstorage's tests, which are run outside of the Netlify platform and the Netlify CLI), and adjust the driver types to ensure compatibility with all supported versions.

See netlify/blobs#183.

I believe @netlify/blobs@8 will error in local dev when using netlify-cli before 17.21.1. This doesn't seem like unstorage's problem though, so I didn't do anything about that here.

The peer dependency only specifies support for v6 and v7, but v8 works
just fine.

This adds v8 to the peer dependencies, bumps the dev dep to v8, updates
the tests (there's a new requirement that's specific to unusual use
cases like unstorage's tests, which are run outside of the Netlify
platform and the Netlify CLI), and adjust the driver types to ensure
compatibility with all supported versions.

See netlify/blobs#183.

I believe @netlify/blobs@8 will error in local dev when using
netlify-cli before 17.21.1. This doesn't seem like unstorage's problem
though, so I didn't do anything about that here.
@@ -33,13 +33,19 @@ export interface NetlifyDeployStoreOptions extends NetlifyBaseStoreOptions {
deployID?: string;
}

export interface NetlifyDeployStoreV8Options extends NetlifyDeployStoreOptions {
// TODO(serhalp) Export this type from @netlify/blobs instead
Copy link
Author

Choose a reason for hiding this comment

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

I'll leave this PR as a draft until I address this.

@@ -39,6 +39,8 @@ describe("drivers: netlify-blobs", async () => {
token,
siteID,
deployID: "test",
// Usually defaulted via the environment; only required in a test environment like this
region: "us-east-1",
Copy link
Member

Choose a reason for hiding this comment

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

Is there a fallback behavior if there is no env & default set?

Copy link
Author

@serhalp serhalp Sep 17, 2024

Choose a reason for hiding this comment

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

No, it bails. But that should only be possible when running outside of the Netlify platform or the Netlify CLI (which means Netlify Blobs wouldn't work anyway... so you wouldn't even get to here), or like I mentioned in the PR description if you're using an old version of the Netlify CLI (<17.21.1).

I'll look into making that error more actionable though. It should ideally tell users to upgrade to netlify-cli@latest if they're running through the CLI, instead of just saying region is missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@netlify/blobs@8 is compatible but is reported as a peer dependency error
2 participants