Skip to content

Commit

Permalink
use model.toJSON to get the request data
Browse files Browse the repository at this point in the history
  • Loading branch information
Mathias Schreck committed Feb 7, 2014
1 parent e1271d9 commit 8c8d7ef
Showing 1 changed file with 3 additions and 10 deletions.
13 changes: 3 additions & 10 deletions shared/syncer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,9 @@ function clientSync(method, model, options) {
}

function serverSync(method, model, options) {
var api, data, urlParts, verb, req;
var api, urlParts, verb, req;

data = _.clone(options.data);
options.url = this.getUrl(options.url, false, data);

This comment has been minimized.

Sign in to view

Copy link
@jeromegn

jeromegn Feb 18, 2014

Contributor

Data was used to build URLs. This was pretty useful.

This comment has been minimized.

Sign in to view

Copy link
@alexindigo

alexindigo Feb 28, 2014

Member

Yep, any specific reason it was nuked? /cc @lo1tuma

This comment has been minimized.

Sign in to view

Copy link
@alexindigo

alexindigo Feb 28, 2014

Member

This change basically renders fetcher:220 useless.

/cc @spikebrehm @c089

This comment has been minimized.

Sign in to view

Copy link
@alexindigo

alexindigo Feb 28, 2014

Member

And it makes client-side and server-side sync methods have different footprints.

This comment has been minimized.

Sign in to view

Copy link
@lo1tuma

lo1tuma Mar 2, 2014

Member

This change was part of #273. data shouldn’t be set via options.
I haven’t understand what this.getUrl should do and why it needs data, so I removed it because:

  • it wasn’t documented
  • there was no test
  • the code wasn’t that expressive to see its intent
options.url = this.getUrl(options.url, false);
verb = methodMap[method];
urlParts = options.url.split('?');
req = this.app.req;
Expand All @@ -70,14 +69,8 @@ function serverSync(method, model, options) {
body: {}
};

/**
* Put the data as form data if POST or PUT,
* otherwise query string.
*/
if (verb === 'POST' || verb === 'PUT') {
api.body = data;
} else {
_.extend(api.query, data);
api.body = _.omit(model.toJSON(), model.idAttribute);
}

req.dataAdapter.request(req, api, function(err, response, body) {
Expand Down

7 comments on commit 8c8d7ef

@bigethan
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit has forced us to write our own fetch() method that puts params into the query string of a request when we're doing the server request.

@lo1tuma
Copy link
Member

@lo1tuma lo1tuma commented on 8c8d7ef Mar 2, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bigethan, @jeromegn you shouldn’t rely on undocumented and untested features.

I haven’t fully understood what interpolateParams should do. Can someone create a new issue / feature request and describe exactly the expected behavior? That would be very helpful.

@jeromegn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lo1tuma That's unfair. Not sure what we were supposed to do. Almost all the code in rendr is undocumented and a lot of it is untested. We have things to get done and one way to figure out how stuff works was to put console.logs everywhere in the module until I got what I wanted.

We want to use rendr. However, lots of it has to be figured out by peering inside the library's code. I personally don't have time to create tests for the untested features I figure out and use. I want to get things done.

I'm not blaming you. I think we should bring that back and explain what it does via tests and documentation (although the examples serve as documentation and use interpolated params)

@lo1tuma
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can understand that. But you have to consider that rendr hasn’t released a stable version yet. Until we release rendr 1.0 there can be a breaking change in every version.
However, the problem here was that I didn’t know about that hidden rendr feature and I was fixing another bug. I removed that feature unintentionally. Since it was not tested I could not realize that I broke it.

As already said, the first step to bring this feature back, would be to create an issue (feature request). Although I personally think such a feature is out of rendr’s scope.

@jeromegn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lo1tuma I agree this is not the responsibility of rendr. It should be handled by the developer, using the url as a function if the attributes are dynamic.

Therefore I'd be more interested in a PR removing the feature entirely (including from the examples obv.).

@rcs
Copy link
Contributor

@rcs rcs commented on 8c8d7ef Mar 24, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lo1tuma A good rule of thumb is to not remove code you don't understand or has interactions you don't understand. If you'd like to have a fully documented, tested code base, rendr is not that starting point. If you'd like to add tests to cover the existing usage of behavior, we could all reason about it moving forward.

Smaller commits that target specific features and changes are a lot easier for people who are using a library to reason about and communicate on. Nothing in the name "Fix client sync to support correct json payload" covers the removal of adding parameters to URLs.

This ended up being an important feature that got broken between a 0.5-rc1 release and the tagged 0.5.

@lo1tuma
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is not a good rule. I think it is better to remove something confusing and reimplement it later in a clean way.

In #271 I described the removal of data.options, so I removed every usage of it even in interpolateParams.

I marked #273 as a breaking change.

Please sign in to comment.