Skip to content

maxUrlLength -> maxURLLength - #3119

Merged
bmac merged 1 commit into
warp-drive-data:masterfrom
sly7-7:fix-maxURLLength-case
Jun 4, 2015
Merged

maxUrlLength -> maxURLLength#3119
bmac merged 1 commit into
warp-drive-data:masterfrom
sly7-7:fix-maxURLLength-case

Conversation

@sly7-7

@sly7-7 sly7-7 commented May 28, 2015

Copy link
Copy Markdown
Contributor

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 😅

@pangratz

Copy link
Copy Markdown
Contributor

@sly7-7 Thanks for the heads up!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@sly7-7

sly7-7 commented May 29, 2015

Copy link
Copy Markdown
Contributor Author

@igorT Done, AppVeyor is angry with npm though...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Might have needed something more for IE8, but I think we are gonna drop it today

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 ?)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We are dropping IE8 so no worries

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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 ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Correct

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, so @igorT Do you want me to wrap that the same way Ember does ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@sly7-7 yes lets do it the way Ember does it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@bmac Ok, Will do this in couple hours. Is this the last thing before merge, or is there any other concerns ?

@bmac bmac added this to the v1.0.0-beta.19 milestone Jun 3, 2015
@sly7-7

sly7-7 commented Jun 4, 2015

Copy link
Copy Markdown
Contributor Author

@bmac Should be good

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be defined on RestAdapter.prototype.

@bmac

bmac commented Jun 4, 2015

Copy link
Copy Markdown
Contributor

Tested it locally and everything looks good. 👍
screen shot 2015-06-04 at 3 36 36 pm

bmac added a commit that referenced this pull request Jun 4, 2015
@bmac
bmac merged commit 5541109 into warp-drive-data:master Jun 4, 2015
@sly7-7
sly7-7 deleted the fix-maxURLLength-case branch June 4, 2015 20:22
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.

Rest Adapter's maxUrlLength is not properly cased

4 participants