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(s3): add S3Storage adapter #220

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

feat(s3): add S3Storage adapter #220

wants to merge 14 commits into from

Conversation

karfau
Copy link

@karfau karfau commented May 5, 2024

  • currently only works for deno, but dependencies ready for node
  • is based on the jsr package @bradenmacdonald/s3-lite-client@0.7.5, which is not available on npm
    • added JSR registry to .npmrc
    • using workspace:* as version specifier to stop pnpm complaining about private workspace packages after adding the JSR registry

https://pnpm.io/workspaces#referencing-workspace-packages-through-aliases
https://jsr.io/docs/npm-compatibility

- currently only works for deno
- is based on the jsr package `@bradenmacdonald/s3-lite-client@0.7.4`, which is not available on npm
@KnorpelSenf KnorpelSenf requested review from rojvv and Satont and removed request for rojvv May 5, 2024 20:14
@Satont
Copy link
Member

Satont commented May 6, 2024

Thanks for contribution.

You can unignore .npmrc and make necessary changes.

But i'm not sure what to do with storage/utils since it's a helper package which doesn't implies publishing it somewhere. Does it have solution? I'm not familiar with jsr.

packages/s3/LICENSE Outdated Show resolved Hide resolved
packages/s3/README.md Outdated Show resolved Hide resolved
packages/s3/test/adapter.test.ts Outdated Show resolved Hide resolved
packages/s3/adapter.ts Outdated Show resolved Hide resolved
packages/s3/adapter.ts Outdated Show resolved Hide resolved
packages/s3/adapter.ts Outdated Show resolved Hide resolved
packages/s3/adapter.ts Outdated Show resolved Hide resolved
@karfau
Copy link
Author

karfau commented May 6, 2024

I will see what I can find regarding the issues with jsr and pnpm workspaces.
But can you tell me which version of pnpm to use?
The current lockfile has version 6. when I user the latest pnpm it upgrades it to version 9 and has tons of changes...

Would it make sense to set the package manager version in package.json, so it can simply be installed using corepack enable?

@Satont
Copy link
Member

Satont commented May 6, 2024

I will see what I can find regarding the issues with jsr and pnpm workspaces. But can you tell me which version of pnpm to use? The current lockfile has version 6. when I user the latest pnpm it upgrades it to version 9 and has tons of changes...

Would it make sense to set the package manager version in package.json, so it can simply be installed using corepack enable?

yea i guess it need to be updated to pnpm 9 version, i'll add changes to main branch. Thanks for pointing it.

@karfau

This comment has been minimized.

@karfau
Copy link
Author

karfau commented May 11, 2024

@Satont I was able to resolve all todos in the code and I think I also managed to cater for all your comments.

If there is anything else required for the PR to land, let me know.

@karfau
Copy link
Author

karfau commented May 29, 2024

Is there an ETA for landing/reviewing this PR, or is there anything I can do to support that it happens?

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.

2 participants