make shouldSerializeHasMany public #2494

[jondayton]

From #2494

[bmac]

Thanks @jondayton. Since this change is going to add a new method to Ember Data's public api would you mind wrapping it in a feature flag?

[jondayton]

@bmac no problem. In that case, since it's removing the private api and adding the public, it may be better to leave the private method and add a public reference under the feature flag

[bmac]

Good plan @jondayton.

[jondayton]

@bmac added a deprecation warning + test. I've been looking at it again and making this a feature flag will require either warnings in several places or a third private method __shouldSerializeHasMany which would sit in the middle to handle the deprecation warning and flag. This seems like a bit of overkill for changing the name of a method, as functionality hasn't really changed, it's just been renamed.

[fivetanley]

I think "The private method _shouldSerializeHasMany has been promoted to the public API. Please remove the underscore to use the public shouldSerializeHasMany method instead." might be a bit better description of what's going on.

[igorT]

Needs a feature flag, we should also make a shouldSerializeBelongsTo as well

[jondayton]

sounds good. I'll take a pass at it tonight.

[bmac]

ping @jondayton do you have time to update this pr?

[jondayton]

@bmac updated the pr with a feature flag.

[jondayton]

logic goes something like this

if feature flag enabled ? use public method : use private method
if private override exists throw a deprecation warning

technically we could break people who are overriding the private method without a deprecation warning, but I do think it's better to throw the warning.

I used shouldSerializeHasMany for the public name, _shouldSerializeHasMany for the private name that is currently being overridden, and __shouldSerializeHasMany for the new private method to use if the feature flag is not enabled

[bmac]

Thanks @jondayton. This looks good. Do you mind rebasing this one last time and I will merge it?

[pangratz]

Unfortunately this doesn't work so the feature is correctly stripped in production. You need to change this to something like:

var shouldSerializeHasMany = '__shouldSerializeHasMany';
if (isEnabled("ds-check-should-serialize-relationships")) {
  shouldSerializeHasMany = 'shouldSerializeHasMany';
}

[bmac]

Sorry @jondayton. @pangratz reminded me that we've been using another pattern for detecting deprecated functions. I added a few comments.

[jondayton]

@bmac was out of town for the long weekend but should have some time tonight.

[jondayton]

@bmac took another pass at it.

[bmac]

Thanks @jondayton