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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nested lists #3282

Closed
wants to merge 34 commits into from

Conversation

Projects
None yet
@JedWatson
Copy link
Member

commented Aug 5, 2016

WIP

This PR adds the long-awaited feature of nested schema support for Keystone 馃帀

See keystonejs/keystone-test-project#17 for example.

To work with this, you'll need to switch over to the new API by removing both instances of /legacy from this file: https://github.com/keystonejs/keystone/blob/master/admin/client/utils/List.js

Yet to be done:

  • Recursive input validation
  • Recursive .updateItem calls (required for field types with async updates)
  • Recursive .getData() calls (required for complex field types)
  • UI is pretty basic

... and probably more.

@JedWatson JedWatson added this to the 0.4.0 milestone Aug 5, 2016

@JedWatson JedWatson self-assigned this Aug 5, 2016

import ItemsTableValue from '../../components/ItemsTableValue';
import { plural } from '../../../admin/client/utils/string';

var ListColumn = React.createClass({

This comment has been minimized.

Copy link
@wmertens

wmertens Aug 5, 2016

Contributor

Why still the createClass and not an ES6 class? Habit?

This comment has been minimized.

Copy link
@JedWatson

JedWatson Aug 8, 2016

Author Member

Honestly, copy + paste... I guess consistency with the rest of the Column classes?

import InvalidFieldType from '../../../admin/client/App/shared/InvalidFieldType';

function generateId () {
return Math.random().toString(36).substr(2, 9);

This comment has been minimized.

Copy link
@wmertens

wmertens Aug 5, 2016

Contributor

I don't understand why this is a random. If you "win the lottery", you get a conflicting id.
Can't it be an monotonously increasing integer, perhaps based on a uuid given by the server if it can't start at 0?

This comment has been minimized.

Copy link
@JedWatson

JedWatson Aug 8, 2016

Author Member

This is just here to give React unique keys for multiple new items in an array.

It's not going to be submitted, and gets reset when you save, so you'd have to be incredibly lucky to get two random keys in that much namespace to conflict. Like, impossible.

Having said that - we could just use an incrementing integer. 馃榿

value.push({
_id: generateId(),
_isNew: true,
});

This comment has been minimized.

Copy link
@wmertens

wmertens Aug 5, 2016

Contributor

The ES6 way to write this would be

const value = [
  ...this.props.value,
  {
    _id: generateId(),
    _isNew: true,
  },
]

which more accurately expresses what you want and thus allows future optimizations.

path: this.props.path,
value: value,
});
},

This comment has been minimized.

Copy link
@wmertens

wmertens Aug 5, 2016

Contributor

The whole function would become

addItem () {
  const {path, value, onChange} = this.props;
  onChange({
    path,
    value: [
      ...value,
      {
        _id: generateId(),
        _isNew: true,
      },
    ],
  });
}
path: this.props.path,
value: value,
});
},

This comment has been minimized.

Copy link
@wmertens

wmertens Aug 5, 2016

Contributor

How about

    removeItem (index) {
        const {value: oldValue, path, onChange} = this.props;
        const value = oldValue.slice(0, index).concat(oldValue.slice(index + 1));
        onChange({ path, value });
    },
handleFieldChange (index, event) {
const value = this.props.value.slice();
const update = value[index];
update[event.path] = event.value;

This comment has been minimized.

Copy link
@wmertens

wmertens Aug 5, 2016

Contributor
  • Are you assuming here that the list schema is only one level deep? Don't we want arbitrarily nested objects?
  • You are mutating that object

I think the field code could become much cleaner by keeping the editor object in Redux and sending changes as (pathArray, changeDescription) tuples. That way, the object editing code is only in one place, and the changes don't need to trickle up.

This list element would not be responsible for patching object changes into itself, and different list implementations would all be able to use that code.

Actually implementing this won't even be that much code, the net effect will probably be less code.

This comment has been minimized.

Copy link
@JedWatson

JedWatson Aug 8, 2016

Author Member

This should work just fine for any level of nesting, unless I've missed something? Each List is self-contained, doesn't know what field types are in it (which could be more list field types)

You're right, mutation slipped past me. Should fix that.

I'm 馃憤 for moving this state into redux, let's do it after (at least) we release the beta. It'll touch all the field types and I don't know for sure there aren't dragons.

This comment has been minimized.

Copy link
@wmertens

wmertens Aug 8, 2016

Contributor

I'm working on a proposal for making the fields pluggable and using Redux etc, for after 0.4 I think.

// props.currentDependencies[dep] = this.state.values[dep];
// });
// }
return React.createElement(Fields[field.type], props);

This comment has been minimized.

Copy link
@wmertens

wmertens Aug 5, 2016

Contributor

More advantages: If you put the edit object in Redux, then a Field would only need path to get at its value, and it could access any values it wanted. So then you would do <fieldType path={[...path, fieldName]} mode="edit" .../> and it would connect() to Redux to get and set its values.

},
renderItems () {
return (
<div>

This comment has been minimized.

Copy link
@wmertens

wmertens Aug 5, 2016

Contributor

We had a lot of code like this in our projects and we came to the conclusion that it is hard to maintain. This function mixes application logic with presentation.

So what we do now, and what we are really happy about, is create a small component which has only one job: to lay out the data calculated here. We give those components a Dom suffix and we generally create them as pure functions in the same file, except if they end up being shared.

So this renderItems() would return an array of

<ItemDom {...{key, name, id, onAdd, onRemove}}>
  {this.renderFieldsForItem(index, value)}
</ItemDom>

and higher up we'd have

const ItemDom = ({name, id, onAdd, onRemove, children}) => (
  <div style...>
    {id && <input type="hidden" name={name} value={id}/>}
    {children}
    buttons...
  </div>
)

(or probably in a shared folder since other Fields need it too)

@@ -0,0 +1,43 @@
/*
TODO: Not sure what this should look like yet,

This comment has been minimized.

Copy link
@wmertens

wmertens Aug 5, 2016

Contributor

I was thinking that nested components should show as indented filters, and they would simply have the filters of their subcomponents. So for an array of Locations you'd be able to filter on street, and you'd find all documents with that street as one of the array values.

That means, you're doing "list contains". Other options would be "only contains", "does not contain", "has at least/at most/exactly x items", 鈥

);
}

function createField (path, options) {

This comment has been minimized.

Copy link
@wmertens

wmertens Aug 5, 2016

Contributor

Can't this somehow be the same function that instantiates "regular" FieldTypes? I think DRY should apply here, also to make recursive types sane.

This comment has been minimized.

Copy link
@JedWatson

JedWatson Aug 8, 2016

Author Member

I thought about that - and would like to clean up the "regular" FieldTypes. Having said that, they do (slightly) different things and this (should) work fine recursively - the main List takes care of top-level fields, this Types.List field type takes over from then on in.

@wmertens wmertens referenced this pull request Aug 13, 2016

Closed

Sub-collections #153

@seet61

This comment has been minimized.

Copy link

commented Jan 11, 2018

Hi! When we can use this feature?

@sheldon-sminq

This comment has been minimized.

Copy link

commented Jan 12, 2018

There is a nested list fork which has a working prototype.

@tobydeh

This comment has been minimized.

Copy link

commented Feb 5, 2018

Why cant i find Types.List in beta 8?

@mherrema

This comment has been minimized.

Copy link

commented Feb 17, 2018

Thanks to all those that have worked on this feature. Unfortunately I'm having an issue uploading multiple Files to a list, and am having trouble with writing a proper fix or finding someone who already has fixed it. Anyone else out there working with multiple S3 files? My error is below.

TypeError: Cannot read property 'id' of undefined

Occurs at list.prototype.updateItem in the async.map. Values appears to be an array before the map, but the "value" coming in is undefined.

@mashaal

This comment has been minimized.

Copy link

commented Feb 17, 2018

@mherrema might be this? #4395

@mherrema

This comment has been minimized.

Copy link

commented Feb 17, 2018

@mashaal thanks - I did have to implement this earlier to fix an error on the front end. Unfortunately it didn't seem to fix the error I'm seeing on save in the backend. Been scouring forks to make sure I have "current" code but haven't seen anything on this in particular.

Update - it appears any updates made to the item will remove the nested files from the item after the update is saved.

@Tathanen

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2018

For anyone looking to use this feature, I've got a branch where I've integrated it with the latest Keystone code (current as of a couple weeks ago) and applied a variety of fixes/tweaks to address items mentioned in this thread.

https://github.com/Tathanen/keystone/tree/nested-lists-modern

Updates of note:

My goal is to have a "modern" version of Keystone with this feature enabled that I can use for a variety of projects, so if anyone notices any further show-stopping issues in this branch I'd be interested to hear about them if only for my own benefit.

The most notable issue that still remains is the inability to nest lists more than one layer deep, nested-nested lists just don't do anything. That's a bit beyond the scope of what I'm comfortable adding, but if you've got any other pressing errors I'd be happy to look into em.

@Norbz

This comment has been minimized.

Copy link

commented Apr 16, 2018

@Tathanen Thanks, this is really helpful !

I tried to use it, but I have one issue that wasn't reported already, so I'm not sure it' not about my configuration.

I have a nested list with a Cloudinary Image in one field (singular). The upload button does not works just after hitting the "add" button.

Right now the workaround is to add an item in the list, hit save, then upload, then save again.

Ill try to find a fix for it, but if you do, let me know

On another hand, this is the most requested feature, I understand that the team is currently working against the technical debt, but even nested lists has a few limitation, I don't get why this isn't being merged in master... There is just so much forks about that it's just silly

@Tathanen

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2018

@Norbz I've been primarily working with S3 uploads, not Cloudinary, but what you're describing seems like it would break at a level that's generic to both. I put something together to test that worked fine for me, lemme know if this kind of setup works for you with Cloudinary or not:

const keystone = require('keystone');
var storage = require( "./S3Storage" );

const { Types } = keystone.Field;

const Thing = new keystone.List('Thing');

Thing.add({
  name: { type: Types.Text, required: true, initial: true },
  pages: {
    type: Types.List,
    fields: {
      fileAttempt: { type: Types.File, storage: storage }
    }
  }
});

Thing.defaultColumns = 'name';

Thing.register();

And then, in that S3Storage file:

var keystone = require('keystone');
var S3Config = keystone.get( "s3 config" );

module.exports = new keystone.Storage({
  adapter: require('keystone-storage-adapter-s3'),
  s3: {
    key: S3Config.key,
    secret: S3Config.secret,
    bucket: S3Config.bucket,
    headers: {
      'x-amz-acl': 'public-read',
    }
  },
  schema: {
    url: true
  }
});

With this I get an admin page where I can click "Add" to add a page, which has a fileAttempt S3 image field. Clicking "upload file" seems to work just fine, and when I save the image indeed gets uploaded to S3. Do you have all these various bits in place for your Cloudinary implementation?

@Norbz

This comment has been minimized.

Copy link

commented Apr 17, 2018

@Tathanen Hey there,

I actually have myself quite the same. Im making a gallery of picture and videos. Videos are stored on S3, and pictures on Cloudinary

I have no issue with the File type. Also works with a localStorage adapter, but for the cloudinary one, when I immediatly click on the upload button, the browse window pops up, I can select my file, but then nothing happens.

I'll try to have a look into that

@Norbz

This comment has been minimized.

Copy link

commented Apr 17, 2018

CloudinaryImage lacks a default value. Some values doesn't get's back to the fields. You can se the same thing in the File field as the uploaded file doesn't features a link, because on init the value.url property isn't there.

That might be because the default value passed is an Array where CloudinaryImage waits for an Object.

Adding the default object value works as a fix. Couldn't see any side effects for now
props.value = value[field.path] || field.defaultValue;

I'm trying to search the reason why value[field.path] isn't correct for CloudinaryImage fields...

@Twansparant

This comment has been minimized.

Copy link

commented Apr 22, 2018

I need this feature badly too! Hope it gets merged soon.
Thanks for your work!

@VinayaSathyanarayana

This comment has been minimized.

Copy link

commented May 3, 2018

Any updates on merging?

@stennie stennie added 4.0 and removed 4.0-release-candidate labels May 31, 2018

@stennie stennie removed this from the 4.0.0 milestone May 31, 2018

@autoboxer autoboxer removed the 4.0 label Jun 2, 2018

@Tathanen

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2018

Lots of back-and-forth on the 4.0 tags, looks like this isn't gonna make it in, eh? Any idea if this'll be a 4.1 fast-follow or is it getting back-burnered for now?

@mahnunchik

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2018

Any news?

@controversial

This comment has been minimized.

Copy link

commented Sep 4, 2018

@JedWatson Any thoughts on when this can be merged? What still needs to be done?

@roonie007

This comment has been minimized.

Copy link

commented Sep 14, 2018

Any news on this ?

@Twansparant

This comment has been minimized.

Copy link

commented Oct 18, 2018

I need this field type soooo badly :(

@stelio

This comment has been minimized.

Copy link

commented Jan 24, 2019

+1

@Vultraz

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2019

Would also be useful here!

@JedWatson

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2019

Sorry everyone, but this isn't going to land in v4. There were too many edge cases and issues for me to merge it into the mainline release.

We've recently opened up our work on Keystone 5 though, which is a completely new architecture designed to make this (and other things) much easier to support in the future.

@Tathanen let me know if you're interested in taking your work on this forward, maybe we can find a way to ship a limited version based on your fork.

@JedWatson JedWatson closed this Apr 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.