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

File fields #1222

Closed
wants to merge 37 commits into from
Closed

File fields #1222

wants to merge 37 commits into from

Conversation

JedWatson
Copy link
Member

@sebmck has contributed a functionally complete (?) implementation of generic File and Files fields.

The job from here is to merge the latest changes in and ensure the fields work correctly across all the use-cases (including the implementation of the backwards compatible field types for LocalFile etc, which will remain until 0.4.0)

Once these are up and running, we'll create branched Image versions of them which will resolve the issues floating around to make File fields play more nicely with images (including previews, etc)

Please sound out here if you're interest / capable of helping with the effort to get this merged!

@etyp
Copy link

etyp commented Mar 17, 2015

I'm in...no doubt 👍

@creynders
Copy link
Contributor

So, I tried to identify which commits have happened in the mean time that affect any of the existing file field/type files. I used:

git log d636282..master --pretty=oneline --follow fields/types/cloudinaryimage
git log d636282..master --pretty=oneline --follow fields/types/cloudinaryimages
git log d636282..master --pretty=oneline --follow fields/types/localfile
git log d636282..master --pretty=oneline --follow fields/types/localfiles
git log d636282..master --pretty=oneline --follow fields/types/s3file
git log d636282..master --pretty=oneline --follow fields/types/Field.js
git log d636282..master --pretty=oneline --follow fields/types/Type.js

To get all commits that affected file field files, then unduped them and this is the result:

f3369d7 Added space += className in CloudinaryImageField.js and added JSX quotes in EmailField.js
601b4e1 ESLint syntax fixes mainly focused on 'semi, quotes' - up tofields/types/code/test/server.js
96318a0 Use filename without suffix as default publicID for cloudinary
ee6f552 Adding displayName to field components
35f74a1 Fixed '' back to "" on JSX attributes
2267490 ESLint syntax clean up up to fields/types/localfile/LocalFileType.js
c5c274f Extract prepost functionality to separate module
2100c01 upload field should collapse
319a9d3 Pass full file object to filename option function
285cc4b Store originalname for localfile docs
c4f820e ESLint syntax clean up - up to: fields/types/relationship/RelationshipType.js
05c6623 ESLint syntax clean up - up to: lib/core/importer.js
cbd341e Revert "Merge pull request #1 from montmanu/s3file-delete"
2dbcfad Revert "Merge pull request #1 from montmanu/s3file-delete"
9dba96d [s3file] stop storing headers in database
2eb38b9 [s3file] remove logging
de5a836 [s3file] use correct _.each() iteratee argument syntax when list is an object
a9b262e [s3file] convert spaces to tabs to reduce diff noise
d769a34 [s3file] check _.isArray and _.isFunction before _.isObject
32eb4db [s3file] support custom headers
03b3416 [s3FileType] delete file on remove, change
7824220 ESLint syntax fixes mainly focused on 'semi, quotes' - up to fields/types/Field.js
d9c91c8 asyncdi is now an independent package
ebd3ec8 Allow value field options to be asynchronous

So, to avoid regressions we need to make sure all of the above changes are merged/migrated to the file-field branch, before we can land this.

@JedWatson @sebmck agreed?

@creynders
Copy link
Contributor

To view a diff of changes to those files:

git difftool d636282..master -- fields/types/cloudinaryimage fields/types/cloudinaryimages fields/types/localfile fields/types/localfiles fields/types/s3file fields/types/Field.js fields/types/Type.js

Maybe it's best to view differences file by file and make sure all applicable changes are transferred to file-fields?

@creynders creynders added the v0.4 label Mar 26, 2015
@creynders
Copy link
Contributor

I'm really sorry, but I'm almost certain there be dragons here, needs confirmation however, went through these just real quick:

  • All pre and post hooks have disappeared for all file fields. confirmed
    • "upload" hooks provided
  • partial regression of 285cc4b, I only found originalname in CloudinaryImageType, but none of the others. confirmed
  • partial regression of ee6f552, no displaynames for any of the file fields.
  • full regression of 2100c01, no shouldCollapse for local file fields
  • full regression of 319a9d3, in fact, AFAICT the whole filename functionality has disappeared? confirmed
  • incorrect implementation of FileType#deleteFile uses undeclared var data
  • downloadFile is not implemented nor declarable.
  • field options are inaccessible in StorageAdapter, adapters
  • allowedTypes filtering
  • multiple files
  • Check whether autoRemove fields really remove the previous file when swapping files
  • catch file upload errors, convert to user-friendly and abort item saving.
  • validateinput
  • updateItem

Have to do with the backwards compatible fields, keeping them here for completeness:

I'll update with confirmed if I'm able to confirm this myself or get notification from someone else, and tick the boxes if solution have been pushed.

@creynders creynders mentioned this pull request Mar 26, 2015
@JedWatson JedWatson modified the milestone: 0.4.0 Apr 28, 2015
@JedWatson JedWatson removed the v0.4 label Apr 28, 2015
@milosdakic
Copy link
Contributor

+1

@felixSchl
Copy link

What's the status of this?

@bawbb
Copy link

bawbb commented Aug 5, 2015

yup. ditto. status?

@creynders
Copy link
Contributor

Status: trailing hopelessly behind. I hope to show this some love next week, since it's sorely needed. But, sooooo much has changed in the mean time and it wasn't easy to begin with ... sigh

@JedWatson
Copy link
Member Author

@creynders if you've got time to pick this up let me know, the only way out is through and this is a huge thing we need to get behind us. Hopefully we'll have some time next week at TM to apply effort specifically to getting this over the line so let's coordinate :)

@creynders
Copy link
Contributor

@JedWatson ready to put my teeth into it again. Will need to do some catching up though ...

@mahnunchik
Copy link
Contributor

Hi @JedWatson @creynders

I would like to help you with this big task.

Could you explain why https://github.com/keystonejs/storage was abandoned?

@morenoh149
Copy link
Contributor

@mahnunchik that was spearheaded by @grabbou and as many oss projects lost steam

@mahnunchik
Copy link
Contributor

@morenoh149 What happened to @grabbou ? If it is not a secret.

@creynders
Copy link
Contributor

@mahnunchik help is more than welcome, thanks! There's a file-fields branch with the changes. Beware though: it's trailing behind a LOT. I just don't seem to find the time to move on with it.

@mahnunchik
Copy link
Contributor

  1. Maybe there is sense to rebase file-fields branch on to of current master before continuing implementation of the file fields?
  2. multer was updated so new version can be used as dependency Fix assertion logic of cookieSecret deprecation message keystone#1563
  3. What do you think about operations on files in memory? For cloud based storage engines it does not require saving files locally. For local files it requires only one filesystem operation. https://github.com/expressjs/multer#memorystorage

@creynders
Copy link
Contributor

Yeah it definitely needs to be rebased first.

@whilefor
Copy link

Any luck with the merge? it is a very useful functionality, need it so much. @creynders @JedWatson

@whilefor
Copy link

Could I just use the branch file-fields for my project?

@morenoh149
Copy link
Contributor

@mahnunchik
Copy link
Contributor

Hi,

I was unsuccessful with the rebase 😢

Someone who will make the rebase should have experience in js and in React too.

@whilefor
Copy link

oh, seems it is a big rebase

@AubreyHewes AubreyHewes mentioned this pull request Jan 7, 2016
@mariohmol
Copy link

+1

@mxstbr
Copy link
Collaborator

mxstbr commented Mar 18, 2016

This is in its current state no longer mergeable, we'll be looking into redoing this with v0.4 based on the current master! (PR anybody? 👍 )

ADMIN NOTE: Please do not delete the branch, we need it as a reference!

@mxstbr mxstbr closed this Mar 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.