Use the new /api/links resource

[seanh]

This is part of hypothesis/product-backlog#215

hypothesis/h#4524 adds a new /api/links endpoint to h.

This pull request replaces the client's serviceUrl service with one that fetches the URL templates from this new endpoint, instead of hardcoding them.

This depends on hypothesis/h#4524 (you will have to checkout the branch of h locally in order to test it) (#4524 is merged now)

With the new serviceUrl the settings.serviceUrl setting is no longer used and can be deleted. This PR doesn't do that yet. A TODO item in hypothesis/product-backlog#215 records that remove the setting still needs to happen.

[seanh]

Update: This wasn't actually an issue, see comment below #356 (comment)

A known issue with this is what happens if the /api/links endpoint is slow:

1. Hack the /api/links endpoint in the h repo to be slow. For example try this diff based off of Add the new /api/links endpoint h#4524's branch:

   diff --git a/h/views/api.py b/h/views/api.py
   index e6badc0..5071861 100644
   --- a/h/views/api.py
   +++ b/h/views/api.py
   @@ -183,6 +183,7 @@ def index(context, request):
                link_name='links',
                description='URL templates for generating URLs for HTML pages')
    def links(context, request):
   +    import time; time.sleep(5);
        group_leave_url = request.route_url('group_leave', pubid='_id_')
        group_leave_url = group_leave_url.replace('_id_', ':id')

2. Hack the client to turn the feature flag on:

   diff --git a/src/sidebar/features.js b/src/sidebar/features.js
   index d53d2be..5098c47 100644
   --- a/src/sidebar/features.js
   +++ b/src/sidebar/features.js
   @@ -21,6 +21,7 @@ function features($log, session) {
       * user yet or if the feature flag name is unknown.
       */
      function flagEnabled(flag) {
   +    return true;
        // trigger a refresh of session data, if it has not been
        // refetched within a cache timeout managed by the session service
        // (see CACHE_TTL in session.js)

Here's the result:

As you can see the links are initially empty strings (href="") and then turn into the correct hrefs after the links response comes back. That's what we want (although we may want to improve the pre-links response state, in a later PR). But the problem is that the links request seems to block the rendering of the UI, and I'm not sure why it's doing that.

Update: This wasn't actually an issue, see comment below #356 (comment)

[seanh]

This is just the original service-url.js, unmodified

[seanh]

The world's simplest reducer. By the way it's easy to generalise this kind of reducer (holds a single object, and updating just means completely replacing the object with a new one) so that the links reducer could be something like: var links = SingleValueReducer('links');

[seanh]

Ugh, I moved service-url.js to legacy-service-url.js, and then put the new serviceUrl at service-url.js, so git is showing it as a modified file, but it's really completely replaced. You may just want to view the service-url.js file, instead of looking at this diff.

[seanh]

Just the original service-url-test.js unmodified

[seanh]

Some comment as with service-url.js, the diff is messed up so I recommend just viewing the service-url-test.js file (which is completely new) instead of looking at the diff

[seanh]

This annotationUI fake is interesting. From serviceUrl's point of view, the contract of annotationUI is just:

• You can set the links value (and it will do the magic re-rendering of everything)
• You can get the links value, and it will return the same object that was set
• Initially, before set, get returns null. Or it could be undefined

This is actually the contract of a standard JavaScript object:

> var annotationUI = {}
> annotationUI.links
undefined
> annotationUI.links = 'foo'
> annotationUI.links
"foo"

I wonder if annotationUI could use getters and setters so that it presents the interface of a normal JavaScript object to serviceUrl? That would simplify serviceUrl's code, but also in the tests fakeAnnotationUI would just be: var fakeAnnotationUI = {};.

[robertknight]

This works for simple actions but not for the case where one action updates multiple parts of the state. For example, the process of logging out might: 1) Update the user's session, 2) Clear the set of annotations, 3) Remove any unsaved drafts. We don't actually have any actions that really do this yet, but it is a common pattern.

btw. What you are describing sounds a bit like MobX which is a library that lets you turn ordinary objects into "observable" ones that trigger arbitrary side-effects then properties are modified.

[seanh]

replaceURLParams could be simpler from the point of view of serviceUrl if it just returned the expanded URL (only) and raised an error if there were unused params. That way serviceUrl wouldn't have to check for unused params and format and throw the error itself. (And it wouldn't have to catch the error either, since it wants to throw it.) But also this mock would just be var replaceURLParams = sinon.stub().returns('EXPANDED_URL') or, when testing what happens when an error is thrown, var replaceURLParams = sinon.stub().throws(new Error()). I haven't looked into how replaceURLParams is used elsewhere, though.

[robertknight]

TBQH I wouldn't have bothered mocking replaceURLParams. On the one hand, doing so means that a change to replaceURLParams implementation cannot break this test.

On the other hand, a) the mock's outputs here don't even resemble the real one, so the test loses some value as documentation. b) the mock's outputs don't depend on the inputs, which creates additional ways for the code under test to do the wrong thing but yet still pass the test.

[seanh]

the mock's outputs don't depend on the inputs, which creates additional ways for the code
under test to do the wrong thing but yet still pass the test.

I don't think this is true (I don't think you can actually change serviceUrl so that it calls replaceURLParams incorrectly, or does the wrong thing with the return value from replaceURLParams, and not have one of my serviceUrl tests fail).

I'm not sure that the test here does lose any value as documentation. All serviceUrl really does is call replaceURLParams and return replaceURLParams's return value. The return value from replaceURLParams is an opaque value, as far as serviceUrl is concerned. The tests, which are tests for serviceURL only after all (replaceURLParams has its own tests elsewhere), do document the intended behaviour of serviceURL correctly (that it just blindly returns what replaceURLParams returns).

I suppose I could have used "https://example.com/expanded_url" instead of "EXPANDED_URL" but I'm not sure there's much difference (this is another case where I would rather have used mock.sentinel.expanded_url if we had that in JavaScript).

There is of course a trade-off that, as you point out, while isolating the serviceUrl tests mean that the serviceUrl tests won't fail if there's a bug in replaceURLParams, if the stub replaceURLParams in service-url-test.js is wrong or out of date then the serviceUrl tests could be passing even though the code is wrong and would crash or misbehave in production. As you point out integrating service-url-test.js with the real replaceURLParams would avoid this problem.

I fall down on the side of isolating the tests here, rather than integrating them, for a couple of reasons.

One is that isolated tests provide feedback on the design of your code by revealing how deeply and complexly the module under test is coupled to its dependencies. If code is tightly coupled you will end up having to create complex mocks. I think that a couple of the mocks I had to create here to write isolated tests for serviceUrl are showing us small examples of unnecessarily tight coupling, which the previous integrated tests for serviceUrl didn't reveal.

The second reason is that it scales better. Across a large test suite when you make a lot of small decisions to integrate the tests for a lot of modules with a lot of their dependencies, then you end up in a situation where running the entire test suite is slow, where a bug in one part of the code causes a cascade of test failures all over the test suite making debugging difficult, and where the design of the code in terms of the coupling, interfaces and contracts of modules is bad because you haven't been getting design feedback from isolated tests.

[robertknight]

Those are sound principles but I'm not convinced they really apply in this case. There is a spectrum of choices when writing tests between injecting absolutely every dependency that your code has and injecting none at all. Both extremes are obviously bad. Injecting a dependency always has a cost in terms of making the code or the tests a bit more complex and less straightforward, as well as differences in behaviour between the injected dependency and the real one.

I usually take the view that pure helper functions which don't involve any substantial computation are not worth injecting most of the time. But in the interests of forward progress, I'm going to ah. disagree and commit.

Let's settle this over a beer in SF 😉

[seanh]

This mock can be deleted when the feature flag is removed

[seanh]

This mock can be deleted when the feature flag is removed

[seanh]

It's a pain that every test that wants to test what serviceUrl does after the links response has been received has to:

1. Extract any variables it needs from this (since this won't be bound in the then() function to follow)
2. return this.linksPromise.then()

It'd be nice if there was a way to encapsulate this in the context() or beforeEach() so it doesn't have to be repeated in every test.

It'd be even nicer if store didn't return promises, although I'm not sure exactly how that would work, but here in serviceUrl we've made use of annotationUI to encapsulate asynchronicity - code that uses serviceUrl doesn't know that there's promises involved. Maybe store could do something similar at least for some use cases

[robertknight]

It'd be nice if there was a way to encapsulate this in the context() or beforeEach() so it doesn't have to be repeated in every test.

I wouldn't worry about it. It isn't that much code and I didn't have any problem understanding what was going on when reading it.

It'd be even nicer if store didn't return promises, although I'm not sure exactly how that would work

store is the API client. It's fundamental job is to make API requests, which need to be async to avoid blocking the UI while the request is happening.

[seanh]

Changing the test's to not use this actually relieved this problem partially as well (see comment below)

[seanh]

I understand that store needs to make API requests asynchronously but I wonder if, at least in some cases, it could immediately return empty results and then update annotationUI with the real result when the response comes back. Depending on what is using store and why, if it's one of these AngularJS components that annotationUI could cause to be re-rendered it might work. I'm sure this would be a big change though and I haven't looked into how store is used / how applicable the idea would even be. Just a thought

[seanh]

I guess this could just be .then(annotationUI.updateLinks)

[robertknight]

Yes, with the caveat that this works provided that updateLinks doesn't depend on the value of this internally - which is the case here IIRC.

[seanh]

Done

[seanh]

Gonna go out for some fresh air. Re the links request blocking rendering of the UI as noted in the comment above, it turns out to have been due to my development h server only serving one request at a time. In dev tools I see both the links and the search requests sent but pending for 5 secs (while the links request is sleeping for 5 secs server-side), then both responses are received when it unblocks. If I add more than one worker to my dev h, then the slow links request no longer blocks rendering of the UI:

[seanh]

Realised that the client can't decide based on a feature flag whether to send the links request or not because the links request is sent before the session / profile response is received - i.e. before the feature flag is received. Is there a simple way to make sending the links request wait until the session response is received? (If I could get a promise for the session response...)

[robertknight]

I would prefer it if the structure here was a little more consistent with the other Redux modules, ie. that update is an object mapping action types to function expressions and the action function is named after what the action does (updateLinks)

[seanh]

I've renamed the functions updateLinks and updateLinksAction:

commit d63373c6c79b1895d11a0df99198fafc02db70e0 (HEAD -> use-new-links-api)
Author: Sean Hammond <git@snhmnd.fastmail.com>
Date:   Tue Apr 18 13:54:12 2017 +0100

    Rename funcs

diff --git a/src/sidebar/reducers/links.js b/src/sidebar/reducers/links.js
index f8993638..d1cf17f8 100644
--- a/src/sidebar/reducers/links.js
+++ b/src/sidebar/reducers/links.js
@@ -13,15 +13,15 @@
 function init() { return {links: null}; }
 
 /** Return updated links based on the given current state and action object. */
-function update(state, action) { return {links: action.newLinks}; }
+function updateLinks(state, action) { return {links: action.newLinks}; }
 
 /** Return an action object for updating the links to the given newLinks. */
-function action(newLinks) {
+function updateLinksAction(newLinks) {
   return { type: 'UPDATE_LINKS', newLinks: newLinks };
 }
 
 module.exports = {
   init:    init,
-  update:  { UPDATE_LINKS: update },
-  actions: { updateLinks: action },
+  update:  { UPDATE_LINKS: updateLinks },
+  actions: { updateLinks: updateLinksAction },
 };

But I'm not sure whether adding an update variable that maps action names to function expressions is worthwhile:

The code before:

/** Return the initial links. */
function init() { return {links: null}; }

/** Return updated links based on the given current state and action object. */
function updateLinks(state, action) { return {links: action.newLinks}; }

/** Return an action object for updating the links to the given newLinks. */
function updateLinksAction(newLinks) {
  return { type: 'UPDATE_LINKS', newLinks: newLinks };
}

module.exports = {
  init:    init,
  update:  { UPDATE_LINKS: updateLinks },
  actions: { updateLinks: updateLinksAction },
};

The code after:

/** Return the initial links. */
function init() { return {links: null}; }


/** Return an action object for updating the links to the given newLinks. */
function updateLinksAction(newLinks) {
  return { type: 'UPDATE_LINKS', newLinks: newLinks };
}

var update = {
  /** Return updated links based on the given current state and action object. */
  UPDATE_LINKS: function (state, action) {
    return {links: action.newLinks};
  },
};

module.exports = {
  init:    init,
  update:  update,
  actions: { updateLinks: updateLinksAction },
};

This may be more consistent with other reducer modules but I think it makes the code longer and harder to read, and also makes the module less consistent with itself (module.exports.actions is an inline literal object, but the module.exports.update object is a variable defined above, the update function is an inline function expression but the action and init functions are named, top-level functions).

I'd vote for keeping module.exports.update as a simple inline literal object?

[robertknight]

This is a nicer way of describing the module than the other modules use (which don't really have a clear scheme). It would be a good idea to make them consistent with this in future (not as part of this PR!)

[seanh]

Sure, I think so too

[robertknight]

Since tests partly serve as documentation, I prefer that test data should look like the real data, even if simplified. eg. If a function with a string argument expects a URL, the test should pass a URL, even if not strictly required for the test to pass.

In this case, the easiest way to do that would be to pass init() as the initial state and say a single-entry routes dictionary as the argument.

[seanh]

I agree that tests partially serve as documentation. But here what I'm attempting to document is that what the UPDATE_LINKS() actually does is blindly take whatever value is passed to it (as newLinks: <VALUE>) and return it (as links: <VALUE>). The actual value, 'NEW_LINKS' in this test, is opaque as far as UPDATE_LINKS() is concerned. It could be a boolean, an object, whatever, and UPDATE_LINKS() would work the same. The fact that what serviceUrl actually passes happens to be an object is not UPDATE_LINKS()'s concern.

I could use an object here instead of a string in order to be consistent with what serviceUrl actually passes here. But then if serviceUrl were changed to store a different type of object these links test would not be failing, but would be out of date. In a weak way, the readability of the links tests would have been coupled to the serviceUrl code, whereas I'd like them to be coupled only to the links code.

Similarly the current value which annotationUI (or whatever part of our Redux machinery) passes to UPDATE_LINKS(), 'CURRENT_STATE' in this test, is also an opaque value (UPDATE_LINKS() actually doesn't touch it at all).

I agree that it's unfortunate that, because the tests use strings here, from reading the tests it might look as if the links reducer is only for storing strings when in fact it's for storing any kind of value and in practice is currently only used to store an object. This is case where in Python you would use a sentinel object rather than a string as a clear way to say this to the reader that this is an opaque value. I don't think any such thing exists in JavaScript so I fell back on all caps strings hoping that would be clear enough.

Even though currently it's only used for storing the links templates, this links reducer could easily be generalised into a "store one opaque value in Redux and be able to update that value by completely replacing it with a new value" reducer, like var linksReducer = singleOpaqueValueReducer('links');.

Do you think that makes sense?

[robertknight]

The fact that the code doesn't care about the shape of links now happens to be true. It might not be the case as the code evolves. I would avoid any abstraction at this stage and add it once there is enough obviously duplicate code to make it worthwhile.

You're also assuming that we keep links as its own state module, as opposed to say, grouping it with other API-related state which we might want to do.

[robertknight]

The convention in our code is to save instances created in setup code as variables rather than assigning to this.

[seanh]

I saw a Stack Overflow answer somewhere that said using this was the "right" way to do this with mocha, but googling just now I can't find anything clearly definitive. If mocha's this has other mocha stuff on it, then using it for our variables might even overwrite something. And also given how unreliable this is in JavaScript I think avoiding it as much as possible is nice. On the other hand this does keep the variables nicely contained with the beforeEach, rather than having to declare them above then set them in beforeEach. Anyway, I'll change it back to our usual style

[seanh]

By not having to work around undesirable this binding in promises, this actually made the tests simpler. It allowed me to remove a lot of var annotationUI = this.annotationUI; rows from it() functions. I'm reminded of the value of avoiding this in JavaScript:

diff --git a/src/sidebar/test/service-url-test.js b/src/sidebar/test/service-url-test.js
index 9c34eb89..de97c1f4 100644
--- a/src/sidebar/test/service-url-test.js
+++ b/src/sidebar/test/service-url-test.js
@@ -40,26 +40,31 @@ function createServiceUrl(linksPromise) {
 
 describe('links', function () {
   context('before the API response has been received', function() {
+    var annotationUI;
+    var replaceURLParams;
+    var serviceUrl;
+    var store;
+
     beforeEach(function() {
       // Create a serviceUrl function with an unresolved Promise that will
       // never be resolved - it never receives the links from store.links().
       var parts = createServiceUrl(new Promise(function() {}));
 
-      this.serviceUrl = parts.serviceUrl;
-      this.store = parts.store;
+      serviceUrl = parts.serviceUrl;
+      store = parts.store;
     });
 
     it('sends one API request for the links at boot time', function() {
-      assert.calledOnce(this.store.links);
-      assert.isTrue(this.store.links.calledWithExactly());
+      assert.calledOnce(store.links);
+      assert.isTrue(store.links.calledWithExactly());
     });
 
     it('returns an empty string for any link', function() {
-      assert.equal(this.serviceUrl('foo'), '');
+      assert.equal(serviceUrl('foo'), '');
     });
 
     it('returns an empty string even if link params are given', function() {
-      assert.equal(this.serviceUrl('foo', {bar: 'bar'}), '');
+      assert.equal(serviceUrl('foo', {bar: 'bar'}), '');
     });
   });
 
@@ -74,33 +79,30 @@ describe('links', function () {
   });
 
   context('after the API response has been received', function() {
+    var linksPromise;
+
     beforeEach(function() {
       // The links Promise that store.links() will return.
-      this.linksPromise = Promise.resolve({
+      linksPromise = Promise.resolve({
         first_link: 'http://example.com/first_page/:foo',
         second_link: 'http://example.com/second_page',
       });
 
-      var parts = createServiceUrl(this.linksPromise);
+      var parts = createServiceUrl(linksPromise);
 
-      this.annotationUI = parts.annotationUI;
-      this.serviceUrl = parts.serviceUrl;
-      this.replaceURLParams = parts.replaceURLParams;
+      annotationUI = parts.annotationUI;
+      serviceUrl = parts.serviceUrl;
+      replaceURLParams = parts.replaceURLParams;
     });
 
     it('updates annotationUI with the real links', function() {
-      var annotationUI = this.annotationUI;
-
-      return this.linksPromise.then(function(links) {
+      return linksPromise.then(function(links) {
         assert.deepEqual(annotationUI.getState(), {links: links});
       });
     });
 
     it('calls replaceURLParams with the path and given params', function() {
-      var serviceUrl = this.serviceUrl;
-      var replaceURLParams = this.replaceURLParams;
-
-      return this.linksPromise.then(function() {
+      return linksPromise.then(function() {
         var params = {foo: 'bar'};
 
         serviceUrl('first_link', params);
@@ -113,10 +115,7 @@ describe('links', function () {
     });
 
     it('passes an empty params object to replaceURLParams if no params are given', function() {
-      var serviceUrl = this.serviceUrl;
-      var replaceURLParams = this.replaceURLParams;
-
-      return this.linksPromise.then(function() {
+      return linksPromise.then(function() {
         serviceUrl('first_link');
 
         assert.calledOnce(replaceURLParams);
@@ -125,9 +124,7 @@ describe('links', function () {
     });
 
     it('returns the expanded URL from replaceURLParams', function() {
-      var serviceUrl = this.serviceUrl;
-
-      return this.linksPromise.then(function() {
+      return linksPromise.then(function() {
         var renderedUrl = serviceUrl('first_link');
 
         assert.equal(renderedUrl, 'EXPANDED_URL');
@@ -135,9 +132,7 @@ describe('links', function () {
     });
 
     it("throws an error if it doesn't have the requested link", function() {
-      var serviceUrl = this.serviceUrl;
-
-      return this.linksPromise.then(function() {
+      return linksPromise.then(function() {
         assert.throws(
           function() { serviceUrl('madeUpLinkName'); },
           Error, 'Unknown link madeUpLinkName');
@@ -145,14 +140,13 @@ describe('links', function () {
     });
 
     it('throws an error if replaceURLParams returns unused params', function() {
-      var serviceUrl = this.serviceUrl;
       var params = {'unused_param_1': 'foo', 'unused_param_2': 'bar'};
-      this.replaceURLParams.returns({
+      replaceURLParams.returns({
         url: 'EXPANDED_URL',
         params: params,
       });
 
-      return this.linksPromise.then(function() {
+      return linksPromise.then(function() {
         assert.throws(
           function() { serviceUrl('first_link', params); },
           Error, 'Unknown link parameters: unused_param_1, unused_param_2');

[robertknight]

I saw a Stack Overflow answer somewhere that said using this was the "right" way to do this with mocha

I can imagine someone saying that. The Mocha docs do make use of this. However my objection to use of this outside of class constructors/methods in JS is that it isn't obvious what the type of this (ie. the constructor and set of valid properties) is.

[robertknight]

You'll get more useful diagnostic messages if you use assert.calledOnce(...) (and other Sinon assert extensions) rather than assert.isTrue(...).

[seanh]

Nice! Done

[robertknight]

I would suggest we at least log the error here (either via console.warn or console.error) so that H developers are more likely to spot that something went wrong.

[seanh]

The reason I didn't bother was that the failed HTTP request is logged in the console anyway (by both Chrome and Firefox). Do you think it's still worth logging our own warning as well?

[robertknight]

One reason it is still worth logging the error here is that it might not be a failed HTTP request. eg. If something fails before or after the call is made.

[seanh]

Done

[robertknight]

Nothing actually ties this code to AngularJS that I can see, so you could just say "A service that..."

[seanh]

Hmm. It is using AngularJS dependency injection, although I realise the caller can just pass in annotationUI and store directly. It's also a little weird that this docstring doesn't actually document the function that it's on, which is a function that returns a function, rather it documents the function that it returns. This feels like an Angular-specific pattern to me (although again, yes, it is just a plain function that returns a function that...). Is this a common pattern, called a "service", that exists beyond just AngularJS?

My other concern is overloading of the word "service" in AngularJS service and the web service and serviceUrl, that's why I said "AngularJS" service to be unambiguous.

[robertknight]

A more generic alternative way of expressing this would be "A factory that returns ...". An AngularJS service is just a class which is instantiated once per application, where the constructor arguments are supplied by the DI system.

[seanh]

The problem is that if I change this docstring to document the outer function: "A factory that returns a function that..." and then document its annotationUI and store params, then that outer docstring (which isn't very useful) will be the one that appears in the docs generated by jsdoc. Even if I do put a docstring on the inner function (and there document the actual linkName and params params) jsdoc does not extract that docstring.

It would work if the inner function was actually defined as function serviceUrl() {...} at top-level, then the outer function could be renamed function serviceUrlFactory() and could still return the inner function, and both serviceUrlFactory and serviceUrl would appear in the jsdoc-generated HTML. Unfortuntely the inner function is a closure...

So the awkwardness remains that the docstring is on the factory function, but actually documents the params of the inner function. The rest of the docstring therefore needs to be a docstring for the inner function as well (so not "A factory that returns...").

Or I change the docstring to document the factory function and its params (annotationUI and store) but also, in the same docstring, document the params of the function that the factory function returns? But I don't think this is the semantics that jsdoc supports.

Anyway, I've left it as a docstring that documents the inner function, not the outer one, but I've changed it to "A function that..." instead of "An AngularJS service..."

[robertknight]

The fact that state updates in the store trigger UI updates is documented in the annotation-ui module itself, so doesn't need to be repeated here - where it might get out of date.

[seanh]

True, removed

[robertknight]

Is there a simple way to make sending the links request wait until the session response is received? (If I could get a promise for the session response...)

You can use session.load() but there might be a simpler alternative. What was the motivation for using a feature flag in the first place?

[seanh]

Is there a simple way to make sending the links request wait until the session response is received? (If I could get a promise for the session response...)

You can use session.load() but there might be a simpler alternative. What was the motivation for using a feature flag in the first place?

As discussed in Slack I think I'm just going to remove the feature flag - the safety net would have been nice to have but isn't worth the cost given that this particular behaviour is difficult to feature flag.

[codecov-io]

Codecov Report

Merging #356 into master will increase coverage by 0.08%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #356      +/-   ##
==========================================
+ Coverage   76.57%   76.65%   +0.08%     
==========================================
  Files         121      122       +1     
  Lines        5981     5993      +12     
  Branches      975      976       +1     
==========================================
+ Hits         4580     4594      +14     
+ Misses       1401     1399       -2

Impacted Files	Coverage Δ
src/sidebar/store.js	92.64% <ø> (ø)	⬆️
src/sidebar/service-url.js	100% <100%> (+14.28%)	⬆️
src/sidebar/annotation-ui.js	100% <100%> (ø)	⬆️
src/sidebar/reducers/links.js	100% <100%> (ø)
src/sidebar/reducers/index.js	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8529299...b40af32. Read the comment docs.

[seanh]

@robertknight Thanks for the helpful review. I think that's all the comments responded to, and many of them done. Let me know what you think of my comments. I think this could do with a manual test from a second person as well, since a lot of changes were made to it today. I'm gonna crash for now

[robertknight]

I've tested this locally and using a browser extension against prod. All looks good. I still have one or two minor reservations about the code but nothing to stop merging. The one comment I would suggest taking a look at @seanh is the one for the "if the API request fails" test.

[robertknight]

Since init() is a pure function with no arguments that returns a trivial value, I think a test is a bit pointless really. It is essentially testing the value of a constant.

[robertknight]

The fact that the code doesn't care about the shape of links now happens to be true. It might not be the case as the code evolves. I would avoid any abstraction at this stage and add it once there is enough obviously duplicate code to make it worthwhile.

You're also assuming that we keep links as its own state module, as opposed to say, grouping it with other API-related state which we might want to do.

[robertknight]

Those are sound principles but I'm not convinced they really apply in this case. There is a spectrum of choices when writing tests between injecting absolutely every dependency that your code has and injecting none at all. Both extremes are obviously bad. Injecting a dependency always has a cost in terms of making the code or the tests a bit more complex and less straightforward, as well as differences in behaviour between the injected dependency and the real one.

I usually take the view that pure helper functions which don't involve any substantial computation are not worth injecting most of the time. But in the interests of forward progress, I'm going to ah. disagree and commit.

Let's settle this over a beer in SF 😉

[robertknight]

This assert is being executed before the Promise handlers inside serviceUrl have actually been executed, so this isn't testing the behaviour you intended to test. The behaviour you actually want to test is difficult without the kind of refactoring we talked about in Slack though to make it possible to wait on the link fetch completing.