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(asset): create asset functionality #210

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

intellix
Copy link
Contributor

@intellix intellix commented May 21, 2019

Largely a work in progress and hadn't seen that you self-assigned it but wanted to push what I had anyway.

Originally I was trying to create another field type with self-contained S3 Upload functionality but then the upload API would need to get merged into Core and I started to think that it needed an Asset type in Core as it would eventually be needed anyway I think.

I haven't connected the S3 part yet but uploading and S3 Uploading is based off of work that I've already done in the past that worked so shouldn't be too far off.

Never really got a chance to discuss how you feel this should be integrated so let me know if it's totally off or if you've already started it or will push it yourself and I'll step back (we just really need it ourselves)

I'm not sure how you'd like to abstract the Storage solutions so I just hardcoded an S3Storage for now inside AssetResolver (maybe a separate module called prime-asset-storage-s3 that when loaded provides an AbstractStorage into Container?)

Partially towards: #209

@codecov
Copy link

codecov bot commented May 21, 2019

Codecov Report

Merging #210 into master will decrease coverage by 0.95%.
The diff coverage is 72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #210      +/-   ##
==========================================
- Coverage   79.34%   78.39%   -0.96%     
==========================================
  Files         108      111       +3     
  Lines        2184     2249      +65     
  Branches      153      170      +17     
==========================================
+ Hits         1733     1763      +30     
- Misses        433      467      +34     
- Partials       18       19       +1
Impacted Files Coverage Δ
...c/modules/internal/repositories/AssetRepository.ts 100% <100%> (ø)
packages/prime-core/src/modules/internal/index.ts 100% <100%> (ø) ⬆️
...rime-core/src/modules/internal/types/AssetInput.ts 100% <100%> (ø)
packages/prime-core/src/entities/Asset.ts 100% <100%> (ø)
packages/prime-core/src/utils/S3Storage.ts 22.22% <22.22%> (ø)
...re/src/modules/internal/resolvers/AssetResolver.ts 72.54% <72.54%> (ø)
...src/modules/external/utils/documentWhereBuilder.ts 48% <0%> (-45.34%) ⬇️
...src/modules/internal/resolvers/DocumentResolver.ts 69.33% <0%> (-3.34%) ⬇️
...es/external/resolvers/createAllDocumentResolver.ts 95.34% <0%> (-2.33%) ⬇️
...ges/prime-field-document/src/PrimeFieldDocument.ts 48.88% <0%> (-2.18%) ⬇️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 097a16c...d2407d8. Read the comment docs.

@birkir
Copy link
Owner

birkir commented May 22, 2019

Wow, you are on fire. Looked at the PR and everything is great so far!

Is S3 nowadays the standard for file bucket APIs? I know google cloud storage has interoperability for AWS S3, but I am thinking for people who use Azure (which is picking up pace).

Should we just go with AWS dependency and worry about other platforms later?

(I was thinking if aws-sdk should be a peerDependecy and make the upload/remove/update part with a replaceable driver interface, but to be honest aws is fine!)

@alfaproject
Copy link

alfaproject commented May 22, 2019

aws-sdk should be a peerDep. That dependency has a new version almost every day, lol

Just add aws-sdk@* on peer deps

@intellix intellix force-pushed the media-asset-s3 branch 3 times, most recently from 13a3578 to 9e6e4c9 Compare May 30, 2019 07:33
@intellix
Copy link
Contributor Author

intellix commented May 30, 2019

S3 is super easy to setup and doesn't require you to go all-in on AWS. Looks something like this: https://medium.com/@shamnad.p.s/how-to-create-an-s3-bucket-and-aws-access-key-id-and-secret-access-key-for-accessing-it-5653b6e54337

I ended up putting aws-sdk: * as a dependency. I'm not sure how you can make it a peer dependency since prime isn't a consumable project where you have the chance to provide your own (unless globally installing on the host)... or is it consumable?

What works:

  • List of assets in a paginated table (I think pagination works?)
  • Mime detection
  • Uploading to S3

What's left:

  • We need to consume the assets in document asset fields, which would probably require some library picker modal. I could do this quite simply in my own Angular projects but feel like I'll end up spending days here
  • The uploading isn't pretty and it doesn't update the table automatically. There's a mix of inline GQL and Stores and I think you want to go back to stores which will probably fix this?
  • Tests

Would love for someone to take this over with a little more React/Ant/Mobx experience :)

Some screenshots of the progress so far:
Screenshot 2019-05-30 15 31 06
Screenshot 2019-05-30 15 45 47

@birkir
Copy link
Owner

birkir commented Jun 10, 2019

I may have messed up your lockfile. If you have any errors, you can just remove it and yarn install again to get a clean one.

Sorry, am trying to keep PR's alive and up to date with master!

@intellix
Copy link
Contributor Author

Rebased against master. Trying to install dependencies in China is extremely frustrating!

@osmanzeki
Copy link

osmanzeki commented Jul 9, 2019

Is S3 nowadays the standard for file bucket APIs? I know google cloud storage has interoperability for AWS S3, but I am thinking for people who use Azure (which is picking up pace).

One great thing about S3 is that it is so prevalant out there that you can talk to other services while still using S3's api.

Example:
https://min.io/

Allows you to host your own buckets while your app still thinks its talking to S3.

@osmanzeki
Copy link

osmanzeki commented Jul 9, 2019

Just to chime in on architecture decisions perhaps for the ability to add more drivers to storage abstraction in the future.

In PHP world, there is a great lib that is mostly universally used for this type of feature:
https://flysystem.thephpleague.com/docs/architecture/

It's basically using an Adapter pattern to allow talking to different types of storage (basically everything out there is supported by Flysystem from cloud providers to in-memory, etc). I think using a similar pattern (one programming API/Interface to talk to all storage types) would be beneficial in the future to add more storage implementations.

This library seems to have reproduced Flysystem in NodeJS if we'd like to have some inspiration for a PrimeCMS/TypeScript version:
https://github.com/Slynova-Org/flydrive

@birkir birkir added the WIP label Jul 18, 2019
@alexkreidler
Copy link

What's the status of this? I'd love to see PrimeCMS become a full-featured solution I can use for production projects.

@intellix
Copy link
Contributor Author

intellix commented Aug 1, 2019

@alexkreidler There's quite a lot done here. It pretty much works and there was a PR against this one that fixes some more stuff on the fork.

I've had to fallback to a hosted CMS (TakeShape) due to time constraints on my current project but would love to continue working on Prime when I have time. If someone wants this they'll have to take over. It's not that far off and just needs tying up

@osmanzeki
Copy link

osmanzeki commented Aug 9, 2019

Further discussion and feedback

Hey folks,

I am currently testing this PR and I wanted to provide some feedback here.

In order to be able to work on the most advanced version of these changes, I forked the PR that was made here.

In my fork, I have also merged the current master branch to make sure I'm not diverging too far from the upstream right now. I'm not sure if this was necessary or not.

My changes

Filesystem Abstraction

Using @slynova/flydrive and S3 compatibility.

I have also tried using node-disks and storage-abstraction but felt like they were both a little bit incomplete api-wise event though I would have prefered a TypeScript-first solution right now.

It is important to note that Flydrive is being rewritten in TypeScript right now. It does not have any type definitions so it can be painful to use in a TS codebase at the moment but this should change with the rewrite.

In order to support MinIO (https://min.io/), Azure Blog Storage, Digital Ocean Spaces and any other S3-compatible storage service, I have added option to provide an S3_REGION, S3_ENDPOINT.

The /packages/prime-core/.env.example file was changed to reflect this in my fork too.

Here are a few examples of how that would work:

# Local (via MinIO instance in S3 "--compat" mode):

S3_REGION=us-east-1
S3_BUCKET=primecms
S3_ENDPOINT=http://localhost:5001
S3_ACCESS_KEY_ID=minio
S3_SECRET_ACCESS_KEY=minio123

# Amazon S3

S3_REGION=us-east-1
S3_BUCKET=primecms
S3_ENDPOINT=https://s3.amazonaws.com
S3_ACCESS_KEY_ID=AKIAIOSFODNN7EXAMPLE
S3_SECRET_ACCESS_KEY=wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY

# Digital Ocean Spaces

S3_REGION=nyc1
S3_BUCKET=primecms
S3_ENDPOINT=https://<region>.digitaloceanspaces.com
S3_ACCESS_KEY_ID=AKIAIOSFODNN7EXAMPLE
S3_SECRET_ACCESS_KEY=wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY

# Azure Blob Storage

S3_REGION=us-east-1
S3_BUCKET=primecms
S3_ENDPOINT=https://<webappname>.azurewebsites.net
S3_ACCESS_KEY_ID=AKIAIOSFODNN7EXAMPLE
S3_SECRET_ACCESS_KEY=wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY

For the local driver right now I store the images in tmp/primecms/storage if the LOCAL_STORAGE_DIRECTORY environment variable is not provided but this is a terrible place since that folder gets deleted when linux is restarted. Just didn't know where to put them and felt like this could be something to discuss in this issue.

https://github.com/osmanzeki/prime/blob/ft/s3-media/packages/prime-core/src/utils/Storage.ts#L59

Error Handling for Asset Uploads

https://github.com/osmanzeki/prime/commit/0711baa35f108f408f3ee6622a430834a34599b8

I've changed and added a few minor things. Notably some error handling for asset uploads when there is something wrong with the values provided in the environment variables for S3.

The toast message now displays the error that occurred (S3-related or otherwise) instead of always displaying a success message like "File uploaded" regardless of the status of the upload.

Other Minor Changes

Desirable future changes and thoughts?

On-the-fly Image Transformations

Since there are talks about having an "on-the-fly image transformation" and/or "media library" features, I think it makes sense to implement the filesystem abstraction layer right away as everything else will be needing it down the line.

Spacechop

For http-drive image transformations, I have tested spacechop but I have had issues with the way it is configured (via a static config.yaml, no programmable api).

I have also had problems with the fact that it considers values like http://localhost:5001/ having a "variable" named :5001 because of the way it is doing regexp in its paths. This is a little bit problematic in some scenarios because in containerized environments, I don't always want to have a reverse proxy just to hide the port numbers in the urls.

Imaginary

I was in the process of testing another similar project called imaginary written in Go. No feedback on this one yet. If it is configurable with a little more flexibility than yaml files, it is already a win in my opinion.

Roll Our Own?

Ideally, it could be great to have something like Glide in the PHP ecosystem but for NodeJS. As far as I know there is nothing as mature as that project out there in the Node ecosystem yet.

The best part of Glide is that it takes care of everything including the http layer, filesystem abstraction (using Flysystem), caching, expiry-headers, security, etc.

Eventually, if this makes sense in the scope of the project one could probably build something like Glide with via NodeJS projects like jimp or sharp.

Related

Prime issues

#209
#101
#207

Other issues

intellix/prime #1
ReyVolver/prime
@slynova/flydrive #87

@osmanzeki
Copy link

osmanzeki commented Aug 25, 2019

The flydrive maintainers have pushed an early release of their 1.0.0 version (the typescript remake, among other changes). I have updated my repo to use the TS version now.

I have also added the ability to delete uploaded images from prime as well as a temporary very basic "AssetsDetail" view that shows up when clicking on an asset row.

Annotation 2019-08-25 000340
Annotation 2019-08-25 000357
Annotation 2019-08-25 000317

I still need to test the "local" storage disk to see how well it behaves. For now, I will be storing the local assets into a folder located at process.cwd()/storage/app/.

Cheers,
Oz

@birkir
Copy link
Owner

birkir commented Aug 28, 2019

This is absolutely fantastic man, your are getting very close here!

Only thing on my todo-list with this PR, are image transformations.

@osmanzeki
Copy link

Indeed. I'm not sure exactly how to approach image transforms. I'm thinking that we can't always do resizes synchronously as that might block the request thread in some conditions. Also, we might have to make sure we're not trying to resize PDFs, Movies, etc. There is nothing currently stopping the user from upload those kind of assets in there (which might be useful for linking to documents in Schemas for sure).

The best solution would most likely be to have another "abstraction layer" for the image transformations so that can be done via Cloudinary and other services of the like if need be. Now that storage is abstracted, it should be relatively straightforward.

As a sidenote, I noticed a few other problems with the current version of the app. It is not storing the user that uploaded the asset properly and is not able to detect the image sizes too. So there is also some work to be done on that end.

@osmanzeki
Copy link

One other thing, I really like how the Spatie Media Library package for Laravel works overall. It allows all kinds of interesting features like placing media in collections, handling uploads, doing transformations, responsive srcset images, getting thumbnails out of videos, etc, etc.

Might be worth it to take some inspirations for database structure and features from that library.

https://docs.spatie.be/laravel-medialibrary/v7/introduction/

@osmanzeki
Copy link

Maybe the easiest way to implement image transforms is to wrap https://github.com/lovell/sharp in a simple HTTP api that can run in Prime and can proxy calls to Cloudinary when needed?

@badoet
Copy link
Contributor

badoet commented Oct 23, 2019

any plan to merge this first before adding the image processing feature?

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

Successfully merging this pull request may close these issues.

6 participants