Remove computed property overrides

[backspace]

This fixes the deprecation warnings described here, from Ember 3.8:
https://deprecations.emberjs.com/v3.x/#toc_computed-property-override

You can see in the Travis logs that the warnings are gone by comparing the log of this most recent run on master, which has 222 instances of “overriden” (sic), to the log of this PR run, which has 0!

I chose to follow a pattern of adding a property to this with the same attribute name with Override appended. This allows the get to check if the overridden property exists before sending the default if it doesn’t. It’s repetitive and a bit unwieldy. Since it’s the same pattern for each computed property, it could be extracted. Let me know if you think that’s desirable or if there are any other changes you’d like to see.

Thanks for this project, it’s great to have around!

[richard-viney]

Great! As you point out, doing this with a computed property macro would end up being less verbose.

[backspace]

Agreed, that’s my preference, but I’m not a maintainer so I’ll wait for confirmation that they’d be into having that kind of abstraction in the codebase 😀

[marcoow]

I think all of the CPs with _ prefixes are only overridden in tests. As they are private API anyway, I'd say it should be fine to replace them with regular properties and set them in init or so which should be much simpler than turning them all into writeable CPs.

[marcoow]

Could this not be replaced with

routeAfterAuthentication: Configuration.routeAfterAuthentication,

simply? I can't remember why this was made a CP in the first place…

[backspace]

I made those changes but maybe getOwner(this) doesn’t work inside init? I’ve run out of time for this at the moment but I’ll see if I can figure it out at some point 😞

[backspace]

There’s more than one thing wrong but it’s true that getOwner won’t work in a mixin because this doesn’t yet exist. I looked at the arguments and the first one for a route mixin is store, so getOwner(store) could work there, but that seems scandalous 😳

(Also, that doesn’t work with createWithContainer elsewhere)

[marcoow]

I think using getOwner(this) in init in a mixin should be fine after this._super(...arguments);. I'm not really sure what the problem is here…

[backspace]

I can’t explain it but I made 4f48367 to add some logging that shows that getOwner(this) is strangely returning false in init, you can see it here in this Travis job. But maybe it’s a red herring, as I think 18db3c4 addressed the failures.

I needed 4379cf3 to get the lint and floating dependencies tests running on Travis, maybe some dependency has dropped support for Node 6? I’ve since removed it, but before that, the tests were passing apart from the FastBoot one but the failures look the same as those on the most recent master job.

I’m unsure about the Node 6 vs 8 thing but could easily open a PR to change that if it’s desirable.

[josemarluedke]

These changes look good! Any change to get them merged soon? Thanks.

[josemarluedke]

I just came across a problem with routeAfterAuthentication not working in an Octane app. Using the changes in this branch in conjunction with changes from master, the problem goes away.

[Turbo87]

closing in favor of #1900

[josemarluedke]

I believe #1900 was just a subset of the changes here. There are a few more deprecations going on.