UI: HTTPAdapter #5637
Merged
UI: HTTPAdapter #5637
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
johncowen
force-pushed
the
feature/ui-http-adapter
branch
from
6d08cf1 to
408605b
Compare
johncowen
force-pushed
the
ui-staging
branch
2 times, most recently
from
60bac11 to
9994267
Compare
This was referenced May 3, 2019
johncowen
force-pushed
the
feature/ui-http-adapter
branch
from
408605b to
1f51d18
Compare
johncowen
force-pushed
the
feature/ui-http-adapter
branch
2 times, most recently
from
af84d90 to
60007f6
Compare
hanshasselberg
approved these changes
Aug 6, 2019
|
🙏 Thanks for the review!
Yup, exactly! As you can see this is a pretty big changeset so didn't want to keep going too far without someone taking a look. I'm gonna try and get this up to date and continue on here now. Probably another PR or 2 to come on top of here to finish up. Thanks again!! |
johncowen
force-pushed
the
feature/ui-http-adapter
branch
2 times, most recently
from
d4de0d7 to
2ee9e7d
Compare
This was referenced Aug 6, 2019
Merged
5 tasks
…5745) * ui: Reconciliate ember-data store when records are deleted via blocking Currently we are barely using the ember-data store/cache, but it will still cache records in the store even though technically we aren't using it. This adds a SyncTime to every record that uses blocking queries so we can delete older records from the ember-data cache to prevent them building up * ui: Add basic timestamp method we can access from tests, fixup tests Adds a timestamp method that we can access from within tests so we can test that the SyncTime is being set. There is probably a better way to do this, but this is also probably the simplest approach - we are also likely to revisit this at a later date
) * ui: Allow text selection of clickable elements and their contents This commit disables a click on mousedown be removing the `href` attribute and moving it to a `data-href` attribute. On mouseup it will only move it back if there is no selection. This means that an anchor will only be followed on click _if_ there is no selection. This fixes the fact that whenever you select some copy within a clickable element it immediately throws you into the linked page when you release your mouse. Further notes: We use the `isCollapsed` property here which 'seems' to be classed as 'experimental' in one place where I researched it: https://developer.mozilla.org/en-US/docs/Web/API/Selection/isCollapsed Although in others it makes no mention of this 'experimental' e.g: - https://webplatform.github.io/docs/dom/Selection/isCollapsed/ - https://w3c.github.io/selection-api/#dom-selection-iscollapsed I may have gone a little overboard in feature detection for this, but I conscious of that fact that if `isCollapsed` doesn't exist at some point in the future (something that seems unlikely). The code here will have no effect on the UI. But I'd specifically like a second pair of eyes on that. * ui: Don't break right click, detects a secondary click on mousedown * ui: Put anchor selection capability behind an ENV var
…5703) EventSources will wait for 1 tick before 'opening'. There is always the chance that the EventSource is '.close()'ed before that tick. We therefore check the 'readyState' before opening the EventSource
…icons` (#6021) 1. Rebuild the heathchecked-resource component now we can copy and paste 2. As the above rebuild came with new icons, we also swapped out 'most' of the other areas where we were using these new icons, plus any icons that were effected by the new icon placeholders 3. Begin to remove more and more of the project specific icons (now replaced by the shared ones)
ember-cli-api-double has been upgraded for 3 things: 1. Use the correct configuration flags 2. Automatically include the necessary files to enable the api doubles without requiring a server. This can be disabled to provide custom functionality (so we can stitch our BDD style testing in with this) 3. When used statically, read the cookies from the users browser to enable basic ad-hoc double editing (e.g. CONSUL_SERVICE_COUNT=100000) Once upgraded we've now moved the config to the correct place, added a new environment (staging) to use the static-style of doubles The test environment continues to use custom cookie setting and url checking so we disable this 'auto importing' by setting 'auto-import' to false for the configuration for the addon. We also added a couple of new package script targets to explicitly serve or build the UI with the entirely static UI.
Initialize session value to `null` to prevent stickiness from a session the previous KV
-Enable blocking queries by default -Change assertion to check for the last PUT request, not just any request for session destruction from a node page. Since we've now turned on blocking queries by default this means that a second GET request is made after the PUT request that we are asserting for but before the assertion itself, this meant the assertion failed. We double checked this by turning off blocking queries for this test using ``` And settings from yaml --- consul:client: blocking: 0 --- ``` which made the test pass again. As moving forwards blocking queries will be on by default, we didn't want to disable blocking queries for this test, so we now assert the last PUT request specifically. This means we continue to assert that the session has been destroyed but means we don't get into problems of ordering of requests here
Right now we only use CONSUL_DOCS_URL, the others are for future usage
- yarn upgrade consul-api-double which includes `status/leader` - add all the ember-data things required to call a new endpoint - Pass the new leader variable through to the template - use the new leader variable in the template to set a leader - add acceptance testing to verify leaders are highlighted - Change testing navigation/api requests to status/leader (on the node listing page, status/leader is now the last get request to be called). - Template whitespace commit (less indenting) - adds a test to to assert no errors happen with an unelected leader
…tc (#6034) This adds the component but doesn't yet use it anywhere. No tests are added here as there isn't an awful lot to test.
- Add netlify redirects file - Add netlify makefile target and commented config settings
|
Furthermore as a result of trying to keep this branch up to date with |
* Migrate queryLeader to new HTTPAdapter * Add some extra tests for queryLeader
|
Tests are passing again here now so this could be merged to Future work at some point in the future (rough notes):
We've bumped this to 'future work' party due to the long running of this PR series, and partly due to the fact that there is enough work done here to allow us to upgrade to a more up to date version of ember. |
johncowen
force-pushed
the
ui-staging
branch
from
74580ab to
d1b7f92
Compare
johncowen
added a commit
that referenced
this pull request
Sep 23, 2019
johncowen
added a commit
that referenced
this pull request
Sep 25, 2019
johncowen
added a commit
that referenced
this pull request
Sep 30, 2019
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
This was referenced Nov 20, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a complete rewrite of the adapter data layer of the Consul UI.
Why
Briefly:
get(this, 'store').query({theQueryObject})) at response time. Without having access to this we have to parse URLs and such like.new EventSource('/v1/internal/ui/services').addEventListener('message')within ember-data.client. The 'client' in theRESTAdapteris more or lessjQuery.ajaxbehind a private method. An injectable HTTP client means we can replace this at some point and opens many other possibilities such as native-like EventSources (via WebWorkers), streaming, caching etc.KV Before: https://github.com/hashicorp/consul/blob/11362e77bebff2e2f66eb23822034d3d0d016bae/ui-v2/app/adapters/kv.js
KV After: https://github.com/hashicorp/consul/blob/6d08cf13ede732f9f6248031546b6ec17c2b6014/ui-v2/app/adapters/kv.js
The more 'nouns' we add to the UI the more of this complicated URL parsing code we have to add.
The important takeaway here is to consider how straightforward each individual Adapter is compared to what it was like previously. Considering the amount of individual Adapters is the thing that grows, its important that these are as simple as possible to create/maintain.
Other
Notes:
trueusing thequeryobject we now have access to, and 2 was actually a bug in the tests that we found that wasn't effecting anything in the app.RESTAdapterjQuery based client in theclient/httprequestmethod for the moment.There are still a few things left to do here, but I've had this hanging around a while and it's getting closer and closer to me actually needing this, so I've PRed it for 👀 while I finish up the following:
Todo:
We'd like to slightly tweak the API so that unserialized and unserialized data is available to the(see ui: Change signature of request/respond methods to take serialized and unserialized data #6285)requestFor...methods, currently only unserialized is.client.request. Usually I wouldn't PR this like this, but the API is pretty much nailed down, we just need to refine the innards slightly and add a little unit testing including making sure headers are encoded etc.Once approved I'll probably move some test files around some of them are now in the wrong place (what were previously adapter tests are now serializer tests), I've avoided moving these for the moment to try to make the already large set of changes easier to view.(see ui: HTTPAdapter - Move Tests #6281 ) Longer term, we could potentially remove a bundle of tests as a lot of the functionality is now massively simpler so its hardly worth testing (we'd be testing string interpolation - which we know works)Lastly, there is probably a similar style serializer refactor coming down the line in a separate PR at some time following this.