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/media store s3 #3124

Merged
merged 9 commits into from
Sep 2, 2022
Merged

Feat/media store s3 #3124

merged 9 commits into from
Sep 2, 2022

Conversation

Phoenix-Alpha
Copy link
Contributor

No description provided.

@Phoenix-Alpha Phoenix-Alpha requested a review from a team as a code owner August 24, 2022 13:30
@changeset-bot
Copy link

changeset-bot bot commented Aug 24, 2022

🦋 Changeset detected

Latest commit: 9333ec6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
next-tinacms-s3 Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Aug 24, 2022

Modified Packages

The following packages were modified by this pull request:

  • next-tinacms-s3

Generated by 🚫 dangerJS against 9333ec6

@vercel
Copy link

vercel bot commented Aug 26, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
tinacms-monorepo ❌ Failed (Inspect) Aug 26, 2022 at 3:59PM (UTC)

@jeffsee55
Copy link
Member

I ran into a couple issues getting this running:

  • First the build wasn't able to find @aws-sdk/signature-v4-crt, it looks like that's a peer dep on @aws-sdk/signature-v4-multi-region.
  • After adding that as a dependency I ran into this error:
    warn  - ../../node_modules/.pnpm/aws-crt@1.14.1/node_modules/aws-crt/dist/native/binding.js
    Critical dependency: the request of a dependency is an expression
    (node:66552) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.
    

@Phoenix-Alpha
Copy link
Contributor Author

@jeffsee55, Can you send me the full error log?

@Phoenix-Alpha
Copy link
Contributor Author

@jeffsee55 ,
Do you still have the issue with running?

@jeffsee55
Copy link
Member

@Phoenix-Alpha yes I'm still seeing this issue, here's the full log:

warn  - ../../node_modules/.pnpm/@aws-sdk+signature-v4-multi-region@3.130.0/node_modules/@aws-sdk/signature-v4-multi-region/dist-cjs/SignatureV4MultiRegion.js
Module not found: Can't resolve '@aws-sdk/signature-v4-crt' in '/Users/jeffsee/code/tinacms2/node_modules/.pnpm/@aws-sdk+signature-v4-multi-region@3.130.0/node_modules/@aws-sdk/signature-v4-multi-region/dist-cjs'

../../node_modules/.pnpm/@aws-sdk+util-user-agent-node@3.127.0/node_modules/@aws-sdk/util-user-agent-node/dist-cjs/is-crt-available.js
Module not found: Can't resolve 'aws-crt' in '/Users/jeffsee/code/tinacms2/node_modules/.pnpm/@aws-sdk+util-user-agent-node@3.127.0/node_modules/@aws-sdk/util-user-agent-node/dist-cjs'
(node:59076) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.
(Use `node --trace-deprecation ...` to show where the warning was created)

@jeffsee55
Copy link
Member

This is happening from within the monorepo, here's my branch: https://github.com/tinacms/tinacms/compare/test-media-store-s3?expand=1

@Phoenix-Alpha
Copy link
Contributor Author

I got the same error but not sure what is the reason.

Actually, I tested using tina-cloud-tester not using experimental example.

@Phoenix-Alpha
Copy link
Contributor Author

Could you test on tina-cloud-starter using this pack? @jeffsee55
tina-cloud-starter.zip

@jeffsee55
Copy link
Member

We'll need this to work for pnpm as well as yarn (which is what the starter uses). I expect we need to add a dependency that's missing from an S3 library.

@Phoenix-Alpha
Copy link
Contributor Author

Phoenix-Alpha commented Aug 31, 2022

Could you push your code used to test DO Space integration? @jeffsee55

@Phoenix-Alpha
Copy link
Contributor Author

@jeffsee55 , I think we can test the s3 integration locally with tina-cloud-starter(the independent repository).
Reference: https://dev.to/scooperdev/use-npm-pack-to-test-your-packages-locally-486e

@jeffsee55
Copy link
Member

Could you push your code used to test DO Space integration? @jeffsee55

I used the monorepo for this and it worked fine.

I got the same error but not sure what is the reason.

I think this is a blocker for this feature. I'm sure it works fine in some contexts but my concern isn't about being able to test it with different environments. The issue is that there's a breakdown in the dependency chain that we need to resolve so users who are using PNPM or Yarn (with Pnp) which are much more strict don't run into issues.

@Phoenix-Alpha
Copy link
Contributor Author

@jeffsee55 , I added @aws-sdk/signature-v4-crt as a devDependency and it worked. Plz check https://github.com/tinacms/tinacms/compare/feat/media-store-s3...test-media-store-s3?expand=1

@jeffsee55
Copy link
Member

devDependency and it worked

I think this will need to be a dependency. It might work in the monorepo because devDependencies are present, but those wouldn't get installed, whereas dependencies would.

@Phoenix-Alpha
Copy link
Contributor Author

@jeffsee55 I changed it to a dependency from devDependency and it still works well. Please confirm.

@jeffsee55
Copy link
Member

@Phoenix-Alpha did you push those changes to this branch?

@Phoenix-Alpha
Copy link
Contributor Author

Phoenix-Alpha commented Sep 2, 2022

@Phoenix-Alpha did you push those changes to this branch?

@jeffsee55, I pushed to this branch - https://github.com/tinacms/tinacms/tree/test-media-store-s3

@jeffsee55
Copy link
Member

This is working for me now, thanks @Phoenix-Alpha! I've updated on this branch so will just merge in once CI passes

@jeffsee55 jeffsee55 merged commit 67b9dd4 into main Sep 2, 2022
@jeffsee55 jeffsee55 deleted the feat/media-store-s3 branch September 2, 2022 19:16
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.

None yet

2 participants