search

carrot / roots-dynamic-content

Dynamic content extension for roots v3
http://roots.cx
Other
33 stars 15 forks source link

change `site` object format #9

Open notslang opened 10 years ago

notslang commented 10 years ago

...not in the sense that JS doesn't handle it - it's malformed in that we're storing content in a property of an array, so it can't even be turned into JSON without breaking:

example from the test dir:

[ { title: 'foo',
    foo: 'bar',
    _content: false,
    _categories: [ 'posts' ],
    _url: '/posts/foo.html' },
  { wow: 'amaze',
    _categories: [ 'posts' ],
    _url: '/posts/locals_test.html',
    content: '{"pretty":true,"filename":"/usrdata/web/roots-dynamic-content/test/fixtures/basic/posts/locals_test.jade","post":{"wow":"amaze","_categories":["posts"],"_url":"/posts/locals_test.html"},"site":{"posts":[{"title":"foo","foo":"bar","_content":false,"_categories":["posts"],"_url":"/posts/foo.html"},{"wow":"amaze","_categories":["posts"],"_url":"/posts/locals_test.html"}]}}' },
  { title: 'other test',
    _render: false,
    _categories: [ 'posts' ],
    _url: '/posts/other_test.html',
    content: '\n<p>another test</p>' },
  nested: [ { title: 'nested 1',
      _categories: [Object],
      _url: '/posts/nested/nested1.html',
      content: '\n<p>nested 1 content</p>' },
    { title: 'nested2',
      _categories: [Object],
      _url: '/posts/nested/nested2.html',
      content: '\n<p>nested 2 contents</p>' },
    'double-nested': [ [Object], [Object] ] ] ]

instead, we should just have the object returned by the all function be the one and only value of site

jescalan commented 10 years ago

I'm not sure I understand the proposed solution?

notslang commented 10 years ago

We'd make the site object formatted like:

[ { title: 'foo',
    foo: 'bar',
    _content: false,
    _categories: [ 'posts' ],
    _url: '/posts/foo.html' },
  { wow: 'amaze',
    _categories: [ 'posts' ],
    _url: '/posts/locals_test.html',
    content: '...' },
  { title: 'other test',
    _render: false,
    _categories: [ 'posts' ],
    _url: '/posts/other_test.html',
    content: '\n<p>another test</p>' },
  { title: 'nested 1',
    _categories: [ 'posts', 'nested' ],
    _url: '/posts/nested/nested1.html',
    content: '\n<p>nested 1 content</p>' },
  { title: 'nested2',
    _categories: [ 'posts', 'nested' ],
    _url: '/posts/nested/nested2.html',
    content: '\n<p>nested 2 contents</p>' },
  { title: 'double-nested 1',
    _categories: [ 'posts', 'nested', 'double-nested' ],
    _url: '/posts/nested/double-nested/double-nested1.html',
    content: '\n<p>double-nested 1 content</p>' },
  { title: 'double-nested 2',
    _categories: [ 'posts', 'nested', 'double-nested' ],
    _url: '/posts/nested/double-nested/double-nested2.html',
    content: '\n<p>double-nested 2 content</p>' } ]

(just like the all() function returns)... so it's all one flat array that you can iterate over. As a side note, this is basically what you'd call a collection if you're working with underscore/backbone. So people should be familiar with this structure.

jescalan commented 10 years ago

Hmm so the issue there is that you would not really be able to pull just one category easily, like site.posts. Do you have an alternate solution for this?

notslang commented 10 years ago

eh... you could use some sort of a helper function in your jade template like underscore provides (_.where / _.find) or you could just do:

each post in site
  - if (post._categories.indexOf('my category') == -1) continue
  //- output the post
jescalan commented 10 years ago

I'd like to have this built in if possible, in the past people have made heavy use of the separate categories. Can you think of any other solutions that might make this a little smoother?

notslang commented 10 years ago

I don't think we can keep the builtin because it isn't a "valid" array, which screws up anything beyond simple iteration. It is a clever hack, but it's not something that we can build on, especially if we add support for templating languages that aren't written in JS.

As for something smoother: I think the ideal solution would be having a way for the user to manipulate the data using a library of their choice in app.coffee. This will probably be needed for https://github.com/jenius/roots/issues/527 anyway, and is actually pretty important for keeping advanced logic out of templates.

jescalan commented 10 years ago

I mean, it is valid javascript, and since roots and its extensions are written in javascript I'm fairly confident that we'll be using javascript templating languages, so I'm not sold on either of those two points.

Is there any specific reason why not being able to transform it into JSON directly (you can still call .all and transform to json) is screwing up something you're working on? I'm trying to get an idea here of whether you are having a compatibility issue with something you're writing, or just brought this up as something you randomly noticed.

notslang commented 10 years ago

Oh, no - it breaks the JSON sterilization in accord-parallel too - when I sterilize it into JSON, it removes all the properties of the array, and then when I make sure that the JSON does contain all the data it's supposed to (which should fail when there are functions included, or other data types that can't be sterilized) _.isEqual says that nothing is missing because underscore assumes that nobody would try putting data in the property of an array.

I might be able to fix accord-parallel with JASON or some clever hack, but the important part of this issue is that arrays shouldn't have hash-map style keys... It isn't a "real" data structure. I do agree that it runs, and it's quite clever, but arrays are meant to have data stored in purely numeric keys (preferably without holes). Properties of arrays should only be reproducible metadata (like .length) or functions (like .pop()).

As for only using JS templating languages - that sounds like an unnecessary limitation to make... accord should be more extensible than that.

jescalan commented 10 years ago

Ok so in the interest of looking further into the root cause of this, could you expand a bit on the background of why you need to sterilize this object? With roots' current implementation, it isn't necessary. Does the output of dynamic content have to be passed through different processes?

Side note: I think we should avoid terms like "nobody would", "shouldn't", "real", and "meant to" in the context they are used here in our discussions. Terms like this are weasel words, and don't add anything to a dialogue other than emphasizing personal opinion, where I'd prefer our discussions to be based more on logical merits.

notslang commented 10 years ago

All objects being passed to another process need to be sterilized into plain text, and since accord-parallel uses worker processes in order to achieve parallel compilation, it also needs sterilization. Roots itself doesn't sterilize anything, but that's not where the problem is - it's in accord-parallel.

And ok, I'll cut back on the anonymous authority:

jescalan commented 10 years ago

Ok, so I see what you are getting after. Not only does the iteration need to be changed, but also the all function you want to eliminate since functions can't be passed through. I just don't really want to drop nesting entirely, and tell users that if they want to actually use nesting, they need to import an external library through app.coffee and use gnarly logic to sort through them. Can you perhaps think of an alternate way that we can support nesting and still have valid json?

As far as the array stuff, still not convinced at all, but I don't think it's relevant. The way other people think things should be used isn't really what's important here, it's how we need things to be to solve problems and continue moving forward, so we can drop that thread for now : )

notslang commented 10 years ago

They shouldn't be using nesting at all - I'm saying they could use some logic in app.coffee to take care of sorting the list of posts, and filtering out categories they want on a per-file basis. And I say that is "ideal" because right now that logic is just put into Jade (where it doesn't belong). It's fine to import a library in app.coffee - that's why we didn't just opt for a simple config file, right? ... we wanted people to be able to use the full power of JS in there.

As for the array stuff - if there's something that doesn't make sense or you think is wrong, I'd be happy to try to explain... The idea isn't to just blindly follow the way other people use arrays, it's to use their methods in a way that helps us solve problems. In this case, avoiding a particular hack makes the program slightly more complex, but will ultimately help us avoid bugs (like the one I mentioned above in accord-parallel).

jescalan commented 10 years ago

Nesting has been very valuable to both us and roots' users in a number of situations, and is an interface feature I'd like to keep by default, not as something users have to manually inject through app.coffee. I fully understand that you do not agree with this, but in this case I'm going to ask you to deal with it and help to come up with a solution here that does include nesting.

If you cannot handle this, and/or are planning on continuing to argue in favor of the approach you have advocated, please come and talk to me in person instead and we can work through it. I want to be extra clear here, if you are planning on posting a comment here in response that continues arguing against nesting, do not do this - come talk to me in person instead.

The bug that you are running into is not a problem with the way the code was written. It's simply the fact that javascript objects are not equal to json. This is not a bug, it's part of how the language is written. You will find there are many other ways in which js objects differ from json. For example, you can include functions in a javascript object, but you cannot with json.

notslang commented 10 years ago

Nesting has been very valuable to both us and roots' users in a number of situations

You keep using that word: "nesting". Let's be clear - I have nothing against the concept of nested data-structures. I do think that a flat collection would be better in this case, but that's not what this issue is about. This issue is about misusing the Array object in a way that is incompatible with JavaScript's design.

I'm going to ask you to deal with it

Telling me to "deal with it" doesn't help me understand why you think structuring data this way is a good idea. Try explaining your reasoning.

Furthermore, just "dealing with it" means ignoring the root of the problem and creating a bigger mess to work around it.

come and talk to me in person instead

Umm, why? I'd be fine with explaining everything I wrote orally, but I'm not sure why you want me to. Also, I tend to prefer documenting conversations as text so I can refer back to them and so design decisions can be done openly.

javascript objects are not equal to json... For example, you can include functions in a javascript object, but you cannot with json.

I am well aware, and I never claimed that they were equal - that's a strawman. I even built accord-parallel with the fact that there are non-sterilizable objects, in mind. That's the whole point of not trying to delegate jobs to workers when the parameters needed for the job cannot be sterilized (a design that I've explained to you more than once).

What I did say is that the data in an array should be sterilizable into JSON... that it can be fully described with JSON, because that's what JSON is: JavaScript Object Notation. It's literally how you write a JavaScript object - you cannot write that an array has non-numeric keys because they aren't supposed to. Not even native methods like concat, or the constructor for the array object, support it.

You are right that functions cannot be described with JSON, but that's because JSON is a data-interchange format, and functions are not a form of data.

Finally, I'd like to revisit your second paragraph. You use the word "arguing", which I find quite disappointing. At this point I've easily spent over an hour trying to explain this situation to you; trying to help you understand. If you merely see this as an argument, then we're doing something wrong. These discussions should be a learning experience - something that will help us avoid making the same mistakes again in the future. Not just a debate.

If it's not helping us improve, then it's not worth my time to explain these things to you. I'd be better off just forking the project and using that derivative wherever I need it.

jescalan commented 10 years ago

I don't know how I could have been more clear in how this would be handled going forward, and you blatantly ignored me. I said it twice, one right after the other, and bolded it both times. This is not acceptable, sean. I'll find a time for us to meet and discuss this shortly.

pruett commented 9 years ago

:two_men_holding_hands: