rendrjs / rendr

Render your Backbone.js apps on the client and the server, using Node.js.
MIT License
4.09k stars 313 forks source link

URI encode interpolated parameter value #514

Closed jmerrifield closed 8 years ago

jmerrifield commented 8 years ago

It seems like there is a discrepancy with how URL encoding takes place.

Given a model var UserModel = BaseModel.extend({urlRoot: '/users'}), we can fetch it with a spec {model: 'UserModel', params: {id: 'президент'}} and the URL fetched will be /users/%D0%BF%D1%80%D0%B5%D0%B7%D0%B8%D0%B4%D0%B5%D0%BD%D1%82 thanks to Backbone's url method applying a encodeURIComponent to the ID.

With a collection var UserOrderCollection = BaseCollection.extend({url: '/users/:user_id/orders'}) and a fetch spec {collection: 'UserOrderCollection', params: {user_id: 'президент'}}, no encoding is applied and so the URL requested is /users/президент/orders.

Upgrading from Node 0.10.30 to a later version has revealed this for us. It seems that newer Node versions might do less automatic escaping of outbound URLs.

This PR attempts to give us consistency over the encoding of dynamically constructed URLs.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 86.054% when pulling 54ac98d68a3f855ff0625e6cb3b3acb842debdbc on jmerrifield:encode-interpolated-params into 827635b189310e6179189a2b531a4b5a252d6179 on rendrjs:master.

sarchila commented 8 years ago

Would it be possible to add a test that would fail on master, but pass on your branch to solidify this issue and the fix?

jmerrifield commented 8 years ago

@sarchila done

jmerrifield commented 8 years ago

We've tested this internally and found that no regressions on Node v0.10.x and that it addresses our problem with later Node versions.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 86.054% when pulling a438105a5169dd6cfac72905a64486c27c97454f on jmerrifield:encode-interpolated-params into 827635b189310e6179189a2b531a4b5a252d6179 on rendrjs:master.

saponifi3d commented 8 years ago

🐑 🇮🇹