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 2.18 > 3.12 #6448

Merged
merged 33 commits into from
Sep 5, 2019
Merged

Conversation

johncowen
Copy link
Contributor

This PR supercedes #6317 and #6406.

As ember-cli-update --run-codemods touches a lot of code (more or less the entire codebase), rebasing/merging the main branch to the above PRs to bring them into sync is far from straightforwards. We decided it would be much easier to re-run the automated ember update/codemods and then cherry-pick the manual amends required for the upgrade on top of that rather than sift through the conflicts.

This PR does that all in one shot (so 2.18 > 3.12).

Please see #6317 and #6406 for details on the more interesting amends required for the upgrade, although the cherry-picked commits here also include reasonably detailed information.

Finally, there is one additional change here, which we believe could be a consequence of a change to RSVP in the upgrade:

Part of the additional features that had been added since the above PRs revolved around adding support for visually highlighting the leader in the node listing. This requires an additional request to a different API endpoint on entering the node listing view (kicked off by findByLeader):

return hash({
items: get(this, 'repo').findAllByDatacenter(dc),
leader: get(this, 'repo').findByLeader(dc),

We have an acceptance tests which tests that when a user clicks a link it takes them to the correct page. It also currently asserts the last GET request made to the API when the user arrives on that page.

Scenario: Clicking [Link] in the navigation takes me to [URL]
When I visit the services page for yaml
---
dc: dc-1
---
When I click [Link] on the navigation
Then the url should be [URL]
Then the last GET request was made to "[Endpoint]"
Where:
-----------------------------------------------------------------------
| Link | URL | Endpoint |
| nodes | /dc-1/nodes | /v1/internal/ui/nodes?dc=dc-1 |
| kvs | /dc-1/kv | /v1/kv/?keys&dc=dc-1&separator=%2F |
| acls | /dc-1/acls/tokens | /v1/acl/tokens?dc=dc-1 |
| intentions | /dc-1/intentions | /v1/connect/intentions?dc=dc-1 |
# | settings | /settings | /v1/catalog/datacenters |
-----------------------------------------------------------------------

Previous to the upgrade the last request was to the status/leader endpoint, after the upgrade the last request changes to the internal/ui/nodes endpoint. We don't know exactly the reasons why the upgrade causes this change, but we are happy enough that it doesn't actually affect anything inportant. Therefore, this PR includes a change to the test to ensure the test passes (ddca6b9).

Notes:

  1. We should probably change this so we are testing all the API endpoints called, without asserting an order (in all cases I can think of the order doesn't matter here, I would imagine that at least in most cases I can't think of - it probably doesn't matter). Basically, loosen up the test slightly so it's less brittle.
  2. As the order doesn't matter here, I'm happy making this change and then addressing the test not to assert the order of things at a later date. This .feature file was one of the first we did to give us maximum coverage/confidence as quickly/easily as possible, so it probably needs completely breaking out now and merging into the more finer grained .feature files.

John Cowen added 30 commits September 4, 2019 15:09
This means that we can't use create anymore to define dynamic methods.

Therefore we dynamically make 2 extended Proxies on demand, and then
create from those. Therefore we can call _super in the init method of
the extended Proxies.
We never actually need to now anyway, this is a remnant of the refactor
from browser based confirmations. We fix it as simply as possible here
but will revisit and remove the old browser confirm functionality at a
later date
We've upgraded ember-data, which changes the code here, which trips the
alarm. We don't use this functionality at all anymore so its safe to
remove the alarm now.
Using a mixture of ES6 classes (and hence super) and arrow functions
means that when babel transpiles the arrow functions down to ES5, a
reference to this is moved before the call to super, hence causing a js
error.

Furthermore, we the testing environment no longer lets use use
apply/call on the constructor.

These errors only manifests during testing (only in the testing
environment), the application itself runs fine with no problems without
this change.

Using ES5 style class definitions give us freedom to do all of the above
without causing any errors, so we reverted these classes back to ES5
class definitions
This test tests a usecase/area of the API that will probably never ever
be used, it was more testing out the API. We've skipped the test for now
as this doesn't affect the application itself, but left a note to come
back here later to investigate further
...to take into account new ember testing style
Initial testing looks like we don't need to call this function anymore,
the function no longer exists
Setting/hanging a computedProperty of an instantiated object no longer
works. Move to setting it on the prototype/class definition instead
New ember API's have changed how you can detect whether something is a
computedProperty or not. It's not immediately clear if its even possible
now. Therefore we change how we detect whether something should be
listened to or not by just looking for presence of `addEventListener`
Basically all the `get`s in application code, and `find`, `getOwner` in
test code.
All our tooling scripts run through a Makefile (for people familiar with
only using those), which then call yarn scripts which can be called
independently (for people familar with only using yarn).

The Makefile targets always check to make sure all the dependencies are
installed before running anything that requires them (building, testing
etc).

The CI scripts/targets didn't follow this same route and called the yarn
scripts directly (usually CI builds a cache of the dependencies first).

For some reason this cache isn't doing what it usually does, and it
looks as though, in CI, ember isn't installed.

This commit makes the CI scripts consistently use the same method as all
of the other tooling scripts (Makefile target > Install Deps if
required > call yarn script). This should install the dependencies if
for some reason the CI cache building doesn't complete/isn't successful.

Potentially this commit may be reverted if, the root of the problem is
elsewhere, although consistency is always good, so it might be a good
idea to leave this commit as is even if we need to debug and fix things
elsewhere.
As we are here making changes for CI purposes (making test-ci
consistent), we spotted that test-parallel is also inconsistent and also
the README manual instructions won't work without `ember` installed
globally.

This commit makes everything consistent and changes the manual
instructions to use the local ember instance that gets installed via
yarn
The codemod seemed to touch this file, but not modify the occurance of
get
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 added the theme/ui Anything related to the UI label Sep 5, 2019
@johncowen johncowen requested a review from a team September 5, 2019 09:34
e9f3133 added this make file target
again, this just removed the duplicate
"scripts": {
"build": "ember build --environment production",
"build:staging": "ember build --environment staging",
"build-ci": "ember build --environment test",
"build:ci": "ember build --environment test",
Copy link
Contributor

Choose a reason for hiding this comment

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

the GNUMakefile still uses yarn run build-ci, do you need to update that too?

Copy link
Contributor Author

@johncowen johncowen Sep 5, 2019

Choose a reason for hiding this comment

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

Oh wow, such a good spot @gregone thanks! I've amended in a commit on the end (I just changed it the one that I left in back to :. Now I can see why it was duplicated in the cherrypick!

Copy link
Contributor

@gregone gregone left a comment

Choose a reason for hiding this comment

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

One question about the makefile, but code-wise, since I assume most changes are automated and everything is properly tested, 👍.

One warning: watch out when removing ember-shims I think it caused some testing issues for us and I had to re-add it

@johncowen
Copy link
Contributor Author

Thanks again for looking at this @gregone

Yeah so this is mainly automated, but there are some manual changes that were already reviewed in the mentioned PRs, the only thing I added here was the test thing I mention in the PR description and then of course the 'makefile target de-duplication' 😆 (oops thanks again for that!) which I spotted post PR.

One warning: watch out when removing ember-shims I think it caused some testing issues for us and I had to re-add it

This was removed by the automated upgrade itself, and tbh I didn't exactly know what it was without looking it up. Seems to be dealing with the old way of importing things in ember? In which case if all my tests pass all should be ok? Just incase though, can you remember how your problems manifested? I'm guessing "tests pass so, 'this is fine'"?

@gregone
Copy link
Contributor

gregone commented Sep 5, 2019

ha, found it. it was in the deprecations warnings, and it seems that there was an issue with ember-cli-flash, which you also seem to use https://github.com/hashicorp/terraform-registry/pull/459

@johncowen
Copy link
Contributor Author

Cool thanks Greg, I took at look at our flashes and they seem to be fine 🤞 . It looks like ember-cli-flash only depends on ember-shims in its devDependencies so we don't pull it in from there. I also did a yarn why ember-shims and nothing came up.

Thanks again for the once over, I think I'm happy to merge this now - I also have #6387 to redo over the top of this. FYI this plan is to keep this hanging in ui-staging for a good few weeks before we put it onto master so it will get plenty of non-automated tyre kicking before then.

@johncowen johncowen merged commit 0e96548 into ui-staging Sep 5, 2019
@johncowen johncowen deleted the feature/ui-ember-update-2 branch September 5, 2019 14:42
johncowen added a commit that referenced this pull request Sep 23, 2019
* ui: Run ember-cli-update (without --to), yarn to update packages, Run ember-cli-update --run-codemods

* ui: Proxies can no longer get away with not calling _super

This means that we can't use create anymore to define dynamic methods.

Therefore we dynamically make 2 extended Proxies on demand, and then
create from those. Therefore we can call _super in the init method of
the extended Proxies.

* ui: We aren't allowed to reset a service anymore

We never actually need to now anyway, this is a remnant of the refactor
from browser based confirmations. We fix it as simply as possible here
but will revisit and remove the old browser confirm functionality at a
later date

* ui: delete the injectableRequestToJQueryAjaxHash alarm test

We've upgraded ember-data, which changes the code here, which trips the
alarm. We don't use this functionality at all anymore so its safe to
remove the alarm now.

* ui: Use set to setup the stub, just normal setting is not allowed

* ui: Revert classes to use ES5 style to workaround babel transp. probs

Using a mixture of ES6 classes (and hence super) and arrow functions
means that when babel transpiles the arrow functions down to ES5, a
reference to this is moved before the call to super, hence causing a js
error.

Furthermore, we the testing environment no longer lets use use
apply/call on the constructor.

These errors only manifests during testing (only in the testing
environment), the application itself runs fine with no problems without
this change.

Using ES5 style class definitions give us freedom to do all of the above
without causing any errors, so we reverted these classes back to ES5
class definitions

* ui: Skip test that seems to have changed due to a change in RSVP timing

This test tests a usecase/area of the API that will probably never ever
be used, it was more testing out the API. We've skipped the test for now
as this doesn't affect the application itself, but left a note to come
back here later to investigate further

* ui: Refactor templated-anchor integration test...

...to take into account new ember testing style

* ui: Remove enumerableContentDidChange

Initial testing looks like we don't need to call this function anymore,
the function no longer exists

* ui: Rework Changeset.isSaving to take into account new ember APIs

Setting/hanging a computedProperty of an instantiated object no longer
works. Move to setting it on the prototype/class definition instead

* ui: Change how we detect whether something requires listening

New ember API's have changed how you can detect whether something is a
computedProperty or not. It's not immediately clear if its even possible
now. Therefore we change how we detect whether something should be
listened to or not by just looking for presence of `addEventListener`

* ui: Manually remove all the imports that codemods don't remove

Basically all the `get`s in application code, and `find`, `getOwner` in
test code.

* ui: Potentially temporary change of ci test scripts to ensure deps exist

All our tooling scripts run through a Makefile (for people familiar with
only using those), which then call yarn scripts which can be called
independently (for people familar with only using yarn).

The Makefile targets always check to make sure all the dependencies are
installed before running anything that requires them (building, testing
etc).

The CI scripts/targets didn't follow this same route and called the yarn
scripts directly (usually CI builds a cache of the dependencies first).

For some reason this cache isn't doing what it usually does, and it
looks as though, in CI, ember isn't installed.

This commit makes the CI scripts consistently use the same method as all
of the other tooling scripts (Makefile target > Install Deps if
required > call yarn script). This should install the dependencies if
for some reason the CI cache building doesn't complete/isn't successful.

Potentially this commit may be reverted if, the root of the problem is
elsewhere, although consistency is always good, so it might be a good
idea to leave this commit as is even if we need to debug and fix things
elsewhere.

* ui: Make test-parallel consistent with the rest of the tooling scripts

As we are here making changes for CI purposes (making test-ci
consistent), we spotted that test-parallel is also inconsistent and also
the README manual instructions won't work without `ember` installed
globally.

This commit makes everything consistent and changes the manual
instructions to use the local ember instance that gets installed via
yarn

* ui: Change make lint target to include hbs linting

* ui: Prevent @@iterator error

* ui: Re-wrangle catchable to fit with new ember 3.12 APIs

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.

* ui: Move page navigation test back to expect node listing first
johncowen added a commit that referenced this pull request Sep 25, 2019
* ui: Run ember-cli-update (without --to), yarn to update packages, Run ember-cli-update --run-codemods

* ui: Proxies can no longer get away with not calling _super

This means that we can't use create anymore to define dynamic methods.

Therefore we dynamically make 2 extended Proxies on demand, and then
create from those. Therefore we can call _super in the init method of
the extended Proxies.

* ui: We aren't allowed to reset a service anymore

We never actually need to now anyway, this is a remnant of the refactor
from browser based confirmations. We fix it as simply as possible here
but will revisit and remove the old browser confirm functionality at a
later date

* ui: delete the injectableRequestToJQueryAjaxHash alarm test

We've upgraded ember-data, which changes the code here, which trips the
alarm. We don't use this functionality at all anymore so its safe to
remove the alarm now.

* ui: Use set to setup the stub, just normal setting is not allowed

* ui: Revert classes to use ES5 style to workaround babel transp. probs

Using a mixture of ES6 classes (and hence super) and arrow functions
means that when babel transpiles the arrow functions down to ES5, a
reference to this is moved before the call to super, hence causing a js
error.

Furthermore, we the testing environment no longer lets use use
apply/call on the constructor.

These errors only manifests during testing (only in the testing
environment), the application itself runs fine with no problems without
this change.

Using ES5 style class definitions give us freedom to do all of the above
without causing any errors, so we reverted these classes back to ES5
class definitions

* ui: Skip test that seems to have changed due to a change in RSVP timing

This test tests a usecase/area of the API that will probably never ever
be used, it was more testing out the API. We've skipped the test for now
as this doesn't affect the application itself, but left a note to come
back here later to investigate further

* ui: Refactor templated-anchor integration test...

...to take into account new ember testing style

* ui: Remove enumerableContentDidChange

Initial testing looks like we don't need to call this function anymore,
the function no longer exists

* ui: Rework Changeset.isSaving to take into account new ember APIs

Setting/hanging a computedProperty of an instantiated object no longer
works. Move to setting it on the prototype/class definition instead

* ui: Change how we detect whether something requires listening

New ember API's have changed how you can detect whether something is a
computedProperty or not. It's not immediately clear if its even possible
now. Therefore we change how we detect whether something should be
listened to or not by just looking for presence of `addEventListener`

* ui: Manually remove all the imports that codemods don't remove

Basically all the `get`s in application code, and `find`, `getOwner` in
test code.

* ui: Potentially temporary change of ci test scripts to ensure deps exist

All our tooling scripts run through a Makefile (for people familiar with
only using those), which then call yarn scripts which can be called
independently (for people familar with only using yarn).

The Makefile targets always check to make sure all the dependencies are
installed before running anything that requires them (building, testing
etc).

The CI scripts/targets didn't follow this same route and called the yarn
scripts directly (usually CI builds a cache of the dependencies first).

For some reason this cache isn't doing what it usually does, and it
looks as though, in CI, ember isn't installed.

This commit makes the CI scripts consistently use the same method as all
of the other tooling scripts (Makefile target > Install Deps if
required > call yarn script). This should install the dependencies if
for some reason the CI cache building doesn't complete/isn't successful.

Potentially this commit may be reverted if, the root of the problem is
elsewhere, although consistency is always good, so it might be a good
idea to leave this commit as is even if we need to debug and fix things
elsewhere.

* ui: Make test-parallel consistent with the rest of the tooling scripts

As we are here making changes for CI purposes (making test-ci
consistent), we spotted that test-parallel is also inconsistent and also
the README manual instructions won't work without `ember` installed
globally.

This commit makes everything consistent and changes the manual
instructions to use the local ember instance that gets installed via
yarn

* ui: Change make lint target to include hbs linting

* ui: Prevent @@iterator error

* ui: Re-wrangle catchable to fit with new ember 3.12 APIs

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.

* ui: Move page navigation test back to expect node listing first
johncowen added a commit that referenced this pull request Sep 30, 2019
* ui: Run ember-cli-update (without --to), yarn to update packages, Run ember-cli-update --run-codemods

* ui: Proxies can no longer get away with not calling _super

This means that we can't use create anymore to define dynamic methods.

Therefore we dynamically make 2 extended Proxies on demand, and then
create from those. Therefore we can call _super in the init method of
the extended Proxies.

* ui: We aren't allowed to reset a service anymore

We never actually need to now anyway, this is a remnant of the refactor
from browser based confirmations. We fix it as simply as possible here
but will revisit and remove the old browser confirm functionality at a
later date

* ui: delete the injectableRequestToJQueryAjaxHash alarm test

We've upgraded ember-data, which changes the code here, which trips the
alarm. We don't use this functionality at all anymore so its safe to
remove the alarm now.

* ui: Use set to setup the stub, just normal setting is not allowed

* ui: Revert classes to use ES5 style to workaround babel transp. probs

Using a mixture of ES6 classes (and hence super) and arrow functions
means that when babel transpiles the arrow functions down to ES5, a
reference to this is moved before the call to super, hence causing a js
error.

Furthermore, we the testing environment no longer lets use use
apply/call on the constructor.

These errors only manifests during testing (only in the testing
environment), the application itself runs fine with no problems without
this change.

Using ES5 style class definitions give us freedom to do all of the above
without causing any errors, so we reverted these classes back to ES5
class definitions

* ui: Skip test that seems to have changed due to a change in RSVP timing

This test tests a usecase/area of the API that will probably never ever
be used, it was more testing out the API. We've skipped the test for now
as this doesn't affect the application itself, but left a note to come
back here later to investigate further

* ui: Refactor templated-anchor integration test...

...to take into account new ember testing style

* ui: Remove enumerableContentDidChange

Initial testing looks like we don't need to call this function anymore,
the function no longer exists

* ui: Rework Changeset.isSaving to take into account new ember APIs

Setting/hanging a computedProperty of an instantiated object no longer
works. Move to setting it on the prototype/class definition instead

* ui: Change how we detect whether something requires listening

New ember API's have changed how you can detect whether something is a
computedProperty or not. It's not immediately clear if its even possible
now. Therefore we change how we detect whether something should be
listened to or not by just looking for presence of `addEventListener`

* ui: Manually remove all the imports that codemods don't remove

Basically all the `get`s in application code, and `find`, `getOwner` in
test code.

* ui: Potentially temporary change of ci test scripts to ensure deps exist

All our tooling scripts run through a Makefile (for people familiar with
only using those), which then call yarn scripts which can be called
independently (for people familar with only using yarn).

The Makefile targets always check to make sure all the dependencies are
installed before running anything that requires them (building, testing
etc).

The CI scripts/targets didn't follow this same route and called the yarn
scripts directly (usually CI builds a cache of the dependencies first).

For some reason this cache isn't doing what it usually does, and it
looks as though, in CI, ember isn't installed.

This commit makes the CI scripts consistently use the same method as all
of the other tooling scripts (Makefile target > Install Deps if
required > call yarn script). This should install the dependencies if
for some reason the CI cache building doesn't complete/isn't successful.

Potentially this commit may be reverted if, the root of the problem is
elsewhere, although consistency is always good, so it might be a good
idea to leave this commit as is even if we need to debug and fix things
elsewhere.

* ui: Make test-parallel consistent with the rest of the tooling scripts

As we are here making changes for CI purposes (making test-ci
consistent), we spotted that test-parallel is also inconsistent and also
the README manual instructions won't work without `ember` installed
globally.

This commit makes everything consistent and changes the manual
instructions to use the local ember instance that gets installed via
yarn

* ui: Change make lint target to include hbs linting

* ui: Prevent @@iterator error

* ui: Re-wrangle catchable to fit with new ember 3.12 APIs

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.

* ui: Move page navigation test back to expect node listing first
johncowen added a commit that referenced this pull request Sep 30, 2019
## HTTPAdapter (#5637)

## Ember upgrade 2.18 > 3.12 (#6448)

### Proxies can no longer get away with not calling _super

This means that we can't use create anymore to define dynamic methods.
Therefore we dynamically make 2 extended Proxies on demand, and then
create from those. Therefore we can call _super in the init method of
the extended Proxies.

### We aren't allowed to reset a service anymore

We never actually need to now anyway, this is a remnant of the refactor
from browser based confirmations. We fix it as simply as possible here
but will revisit and remove the old browser confirm functionality at a
later date

### Revert classes to use ES5 style to workaround babel transp. probs

Using a mixture of ES6 classes (and hence super) and arrow functions
means that when babel transpiles the arrow functions down to ES5, a
reference to this is moved before the call to super, hence causing a js
error.

Furthermore, we the testing environment no longer lets use use
apply/call on the constructor.

These errors only manifests during testing (only in the testing
environment), the application itself runs fine with no problems without
this change.

Using ES5 style class definitions give us freedom to do all of the above
without causing any errors, so we reverted these classes back to ES5
class definitions

### Skip test that seems to have changed due to a change in RSVP timing

This test tests a usecase/area of the API that will probably never ever
be used, it was more testing out the API. We've skipped the test for now
as this doesn't affect the application itself, but left a note to come
back here later to investigate further

### Remove enumerableContentDidChange

Initial testing looks like we don't need to call this function anymore,
the function no longer exists

### Rework Changeset.isSaving to take into account new ember APIs

Setting/hanging a computedProperty of an instantiated object no longer
works. Move to setting it on the prototype/class definition instead

### Change how we detect whether something requires listening

New ember API's have changed how you can detect whether something is a
computedProperty or not. It's not immediately clear if its even possible
now. Therefore we change how we detect whether something should be
listened to or not by just looking for presence of `addEventListener`

### Potentially temporary change of ci test scripts to ensure deps exist

All our tooling scripts run through a Makefile (for people familiar with
only using those), which then call yarn scripts which can be called
independently (for people familar with only using yarn).

The Makefile targets always check to make sure all the dependencies are
installed before running anything that requires them (building, testing
etc).

The CI scripts/targets didn't follow this same route and called the yarn
scripts directly (usually CI builds a cache of the dependencies first).

For some reason this cache isn't doing what it usually does, and it
looks as though, in CI, ember isn't installed.

This commit makes the CI scripts consistently use the same method as all
of the other tooling scripts (Makefile target > Install Deps if
required > call yarn script). This should install the dependencies if
for some reason the CI cache building doesn't complete/isn't successful.

Potentially this commit may be reverted if, the root of the problem is
elsewhere, although consistency is always good, so it might be a good
idea to leave this commit as is even if we need to debug and fix things
elsewhere.

### Make test-parallel consistent with the rest of the tooling scripts

As we are here making changes for CI purposes (making test-ci
consistent), we spotted that test-parallel is also inconsistent and also
the README manual instructions won't work without `ember` installed
globally.

This commit makes everything consistent and changes the manual
instructions to use the local ember instance that gets installed via
yarn

### Re-wrangle catchable to fit with new ember 3.12 APIs

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.

### Build bindata_assetfs.go with new UI changes
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