maxUrlLength -> maxURLLength #3119
Conversation
|
@sly7-7 Thanks for the heads up! |
| maxUrlLength: 2048, | ||
| maxURLLength: 2048, | ||
|
|
||
| maxUrlLength: Ember.computed(function() { |
There was a problem hiding this comment.
Shouldn't turn it into a computed if even we weren't calling get on it. You can deprecate using deprecateProperty/making a prop getter similar to https://github.com/emberjs/data/blob/master/packages/ember-data/lib/system/snapshot.js#L39
|
@igorT Done, AppVeyor is angry with npm though... |
| @@ -838,3 +838,13 @@ function endsWith(string, suffix) { | |||
| return string.endsWith(suffix); | |||
| } | |||
| } | |||
|
|
|||
| Ember.defineProperty(RESTAdapter, 'maxUrlLength', { | |||
There was a problem hiding this comment.
Might have needed something more for IE8, but I think we are gonna drop it today
There was a problem hiding this comment.
Sorry, I missed that. What could we do for IE8 ? I can at least put this defineProperty inside a Ember.platform.hasPropertyAccessors block, but how could I do it for IE8 ? (Am I missing something or Ember.deprecateProperty is just a noop on IE8 ?)
There was a problem hiding this comment.
We are dropping IE8 so no worries
There was a problem hiding this comment.
Also wouldn't this break if someone subclassed the adapter to provide their own limit as they would override this getter? cc @rwjblue whats the proper way of doing this
There was a problem hiding this comment.
Here is the code Ember uses to deprecate a property. https://github.com/emberjs/ember.js/blob/1b5adaa65d20af4f4e57702ea34aa0dcc42e1489/packages/ember-metal/lib/deprecate_property.js#L24-L43. It seems to work even if a user overrides this getter. Its also usually called on the classes's prototype object.
There was a problem hiding this comment.
@bmac I saw this too, so that basically mean that people developping in a world without property accessors, they just don't have the deprecation warning, right ?
There was a problem hiding this comment.
Ok, so @igorT Do you want me to wrap that the same way Ember does ?
There was a problem hiding this comment.
@sly7-7 yes lets do it the way Ember does it.
There was a problem hiding this comment.
@bmac Ok, Will do this in couple hours. Is this the last thing before merge, or is there any other concerns ?
|
@bmac Should be good |
| @@ -838,3 +838,15 @@ function endsWith(string, suffix) { | |||
| return string.endsWith(suffix); | |||
| } | |||
| } | |||
|
|
|||
| if (Ember.platform.hasPropertyAccessors) { | |||
| Ember.defineProperty(RestAdapter, 'maxUrlLength', { | |||
There was a problem hiding this comment.
This should be defined on RestAdapter.prototype.
|
Tested it locally and everything looks good. 👍 |
Fixes #3101
@igorT Let me know if I can remove the deprecation warning (it seems to me that people may override groupsForHasMany and use this property, like in the test)
@pangratz When this is merged, you will have to rebase your PR #3099 😅