Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ui: Ember upgrade 3.8 > 3.12 #6406

Closed
wants to merge 8 commits into from

Conversation

johncowen
Copy link
Contributor

This PR uses ember-cli-update to upgrade from ember 3.8 to ember 3.12.

We also hit one or two interesting problems after doing the upgrade.

Everything here apart from the changes to catchable.js are automated changes.

In the upgrade from ember 3.8 > 3.12 the public interfaces for
ComputedProperties have changed slightly. meta is no longer a public
property of ComputedProperty but of a ComputedDecoratorImpl mixin
instead.

https://github.com/emberjs/ember.js/blob/7e4ba1096e3c2e3e0dde186d5ca52ff19cb8720a/packages/%40ember/-internals/metal/lib/computed.ts#L725

There seems to be no way, by just using publically available
methods, to replicate this behaviour so that we can create our own
'ComputedProperty` factory via injecting the ComputedProperty class as
we did previously.

/**
* Gives you factory function to create a specified type of ComputedProperty
* Largely taken from https://github.com/emberjs/ember.js/blob/v2.18.2/packages/ember-metal/lib/computed.js#L529
* but configurable from the outside (IoC) so its reuseable
*
* @param {Class} ComputedProperty - ComputedProperty to use for the factory
* @returns {function} - Ember-like `computed` function (see https://www.emberjs.com/api/ember/2.18/classes/ComputedProperty)
*/
export default function(ComputedProperty) {
return function() {
const args = [...arguments];
const cp = new ComputedProperty(args.pop());
if (args.length > 0) {
cp.property(...args);
}
return cp;
};
}

Instead we dynamically hang our Catchable catch method off the
instantiated ComputedProperty. In doing it like this ComputedProperty
has already has its meta method mixed in so we don't have to manually
mix it in ourselves (which doesn't seem possible).

The utils/computed/factory util is now useless so we've removed it.

This functionality is only used during our work in trying to ensure
our EventSource/BlockingQuery work was as 'ember-like' as possible (i.e.
using the traditional Route.model hooks and ember-like Controller
properties). Our ongoing/upcoming work on a componentized approach to
data a.k.a <DataSource /> means we will be able to remove the majority
of the code involved here now that it seems to be under an amount of
flux in ember.

John Cowen added 7 commits August 27, 2019 09:14
In the upgrade from ember 3.8 > 3.12 the public interfaces for
ComputedProperties have changed slightly. `meta` is no longer a public
property of ComputedProperty but of a ComputedDecoratorImpl mixin
instead.

https://github.com/emberjs/ember.js/blob/7e4ba1096e3c2e3e0dde186d5ca52ff19cb8720a/packages/%40ember/-internals/metal/lib/computed.ts#L725

There seems to be no way, by just using publically available
methods, to replicate this behaviour so that we can create our own
'ComputedProperty` factory via injecting the ComputedProperty class as
we did previously.

https://github.com/hashicorp/consul/blob/3f333bada181aaf6340523ca2268a28d1a7db214/ui-v2/app/utils/computed/factory.js#L1-L18

Instead we dynamically hang our `Catchable` `catch` method off the
instantiated ComputedProperty. In doing it like this `ComputedProperty`
has already has its `meta` method mixed in so we don't have to manually
mix it in ourselves (which doesn't seem possible)

This functionality is only used during our work in trying to ensure
our EventSource/BlockingQuery work was as 'ember-like' as possible (i.e.
using the traditional Route.model hooks and ember-like Controller
properties). Our ongoing/upcoming work on a componentized approach to
data a.k.a `<DataSource />` means we will be able to remove the majority
of the code involved here now that it seems to be under an amount of
flux in ember.
@johncowen johncowen requested a review from a team August 27, 2019 14:00
@johncowen johncowen added blocks-release Issue must be addressed or downgraded before the current milestone can be released. theme/ui Anything related to the UI and removed blocks-release Issue must be addressed or downgraded before the current milestone can be released. labels Aug 27, 2019
Copy link

@backspace backspace left a comment

Choose a reason for hiding this comment

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

Nice and easy! I noticed a huge amount of noise in the logs from this deprecation warning:

Model.data was private and it's use has been deprecated. For public access, use the RecordData API or iterate attributes [deprecation id: ember-data:Model.data]

I don’t know what your approach is to deprecations but if it’s something you’re planning to leave in for now, you could check out ember-cli-deprecation-workflow to silence the message until you’re ready to deal with it. I’m not 100% sure but it seems like that message might be causing Circle to be unable to render the entire log!

ui-v2/app/computed/catchable.js Outdated Show resolved Hide resolved
@johncowen
Copy link
Contributor Author

Hey @backspace!

Thanks again for looking over this!

I noticed a huge amount of noise in the logs from this deprecation warning

😂 yeah there are loads of them. For me deprecations straight after an upgrade are 'fine' (more or less), with the understanding that deprecations are 'we are going to be changing this soon so between now and another upgrade in the future, you should probably change this before you do another big upgrade'.

In saying that, I don't like all this noise at all myself and will be looking at these way before we actually need to. But I don't want them to block a merge as deprecations are still 'ok' (kinda). If I can get this run of work in before fixing the deprecations I will (but while I'm waiting I'll be looking into them at least, so expect more PRs for your 👀 ! )

I'll have a look at that addon you linked to, it definitely could be useful here. Generally I like things bugging me until they are sorted out, it means I don't forget about them, so I don't really want things hiding completely, but it would be nice to just see them in the CLI instead of in console also - will take a look 🙇

Lastly if it was just that failing test that was blocking approval, that should be ok now 🤞

@backspace backspace self-requested a review August 27, 2019 15:01
@johncowen
Copy link
Contributor Author

Closing in favour of #6448

@johncowen johncowen closed this Sep 5, 2019
@johncowen johncowen deleted the feature/ui-ember-3-12 branch September 12, 2019 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants