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

Added support for "before" for Memory and Rest #22

Closed
wants to merge 1 commit into from
Closed

Added support for "before" for Memory and Rest #22

wants to merge 1 commit into from

Conversation

mercmobily
Copy link

Hi there,

This implements #6 for Memory.js and for Rest.js.
Two issues:

In Rest.js I set "null" for the X-rest-before header. Ideally, the header should be left empty (that is, when the server sees that the X-rest-before header is there but it's empty, it means "place at the end"). However, Dojo doesn't allow empty headers (they are taken out):

https://github.com/dojo/dojo/blob/master/request/xhr.js#L205-L212

In Cache.js, in put() an item is always deleted and then added again:

https://github.com/SitePen/dstore/blob/master/Cache.js#L149-L158

This means that even though my implementation of Memory moves as little as possible, it ends up always moving the item from its last position when using Memory.js as the caching store

Thanks for listening as ever!

Merc.

@brandonpayton
Copy link
Contributor

Hi @mercmobily, I was hoping to work with you on this PR, but after looking at the other things we'd need to do to support options.before, it seemed like the most efficient thing was to just do it. The work is pushed to the options-before branch of the dstore repo. It's there so Kris could offer a second pair of eyes on some of the more critical pieces.

I ended up changing the option to options.beforeId since the ID is the only thing actually required, Store, Memory, Rest, and Observable are updated to support it.

For Rest, we decided to use a combination of headers:

  • X-Put-Before - sent only when options.beforeId !== null
  • X-Put-Default-Position -
    • set to 'top' when collection.defaultToTop === true
    • set to 'bottom' when collection.defaultToTop == false or options.beforeId === null

These things are subject to change since they haven't been reviewed yet, but I thought it was worth sharing what has been done so far.

Thanks for your work and all the dialogue.

@mercmobily
Copy link
Author

Hi,

What is the best spot to discuss this? This PR is closed...

Merc.
On 10/06/2014 3:39 PM, "Brandon Payton" notifications@github.com wrote:

Hi @mercmobily https://github.com/mercmobily, I was hoping to work with
you on this PR, but after looking at the other things we'd need to do to
support options.before, it seemed like the most efficient thing was to
just do it. The work is pushed to the options-before branch of the dstore
repo. It's there so Kris could offer a second pair of eyes on some of the
more critical pieces.

I ended up changing the option to options.beforeId since the ID is the
only thing actually required, Store, Memory, Rest, and Observable are
updated to support it.

For Rest, we decided to use a combination of headers:

  • X-Put-Before - sent only when options.beforeId !== null
  • X-Put-Default-Position -
    • set to 'top' when collection.defaultToTop === true
    • set to 'bottom' when collection.defaultToTop == false or options.beforeId
      === null

These things are subject to change since they haven't been reviewed yet,
but I thought it was worth sharing what has been done so far.

Thanks for your work and all the dialogue.


Reply to this email directly or view it on GitHub
#22 (comment).

@kriszyp
Copy link
Contributor

kriszyp commented Jun 10, 2014

I PR'ed his branch, for your convenience: #29

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

3 participants