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: UI Release Merge (ui-staging merge) #6527

Merged
merged 4 commits into from
Sep 30, 2019
Merged

ui: UI Release Merge (ui-staging merge) #6527

merged 4 commits into from
Sep 30, 2019

Conversation

johncowen
Copy link
Contributor

@johncowen johncowen commented Sep 23, 2019

This PR is a 'ui release' from ui-staging. All commits (apart from the final commit) have corresponding approved PRs and have already been individually squash merged to ui-staging.

This merge itself will not be squash merged in order to maintain PR history.

The final commit has not been approved previously but is a result of running make ui to ensure anyone building consul from master gets these UI changes.

@johncowen johncowen requested a review from a team September 23, 2019 16:17
@johncowen johncowen added the theme/ui Anything related to the UI label Sep 23, 2019
@alvin-huang
Copy link
Collaborator

Makefile and ember exam changes all LGTM. Just a small nit pick but your commit message has a misspelling, might be easier to grep in the future if you change it to assetfs instead of assetsfs

@johncowen
Copy link
Contributor Author

😆 whoopsie! Cool thanks Alvin thats that done also.

@johncowen
Copy link
Contributor Author

Actually @alvin-huang , I'm kind of working my way towards a consistent way of doing these, is there anything else I can do with these type of merges that might be handy?

@alvin-huang
Copy link
Collaborator

@johncowen What is 'these' referring to? UI updates?

@johncowen
Copy link
Contributor Author

johncowen commented Sep 23, 2019

Oh sorry yeah, one sec let me back up a bit.

So this PR merge is from my 'staging' branch to master, so a collection of PR's I've been doing over the last few weeks or months.

I realised this time around that it would be a good idea to run a make ui and add a commit of the result of that onto the end of the PR that anyone building from master would get the updated UI in their build seeing as the assetfs file is kind of like the artifact produced by a UI 'release'

When you mentioned being able to grep the git log for assetfs it got me wondering if there's anything else could do with these types of PRs, for example for any future searching or to provide historical information etc. I always try to add a ui: to the front of the commit messages for ui PRs but maybe we could add something different for the build of assetfs, and I could try to make a point of consistently doing that also? I dunno, maybe I'm overthinking things.

@johncowen johncowen force-pushed the ui-staging branch 3 times, most recently from b395fd2 to a3e543c Compare September 25, 2019 19:26
@alvin-huang
Copy link
Collaborator

Ah, adding UI in front would be fine or omitting it would be fine too. In general, no one will be PRing that file nor modifying it manually so we shouldn't have to trace the history of it outside of 'build the current JS files and commit the go file'.

Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

Seems fine to me. I didn't look at anything in ui-v2 specifically but overall this seems fine.

johncowen and others added 4 commits September 30, 2019 13:27
* 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
* yarn upgrade

* Manually upgrade all dependencies to latest versions

* ui: Switch lint errors to warnings so we can look at them later

* ui: Upgrade our custom href-to helper for new href-to upgrade
@johncowen johncowen merged commit b3b32dc into master Sep 30, 2019
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.

3 participants