ui: update fetch-mock

[tamird]

This change is

[maxlang]

You didn't replace all the describe functions.
Why don't they need a proper closures anymore?

[maxlang]

Mostly

We should get rid of the Object.assign global polyfill except for tests and make sure that changing all the tests to use arrow functions is the correct move.

---

Review status: 0 of 15 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

---

ui/app/redux/ui.ts, line 31 [r2] (raw file):

    case SET_UI_VALUE:
      let { payload } = action as PayloadAction<UISetting>;
      return Object.assign({}, state, {[payload.key]: payload.value});

We should probably stop using Object.assign here like we have in the rest of the code.

New code would be:

state = _.clone(state);
state[payload.key] = payload.value;
return state;

---

ui/app/util/object-assign.ts, line 1 [r2] (raw file):

interface ObjectConstructor {

We should add a comment at the top explaining why this is necessary, since none of our code will use it directly if we remove it from ui.ts. I'm almost in favor of just adding it to the fetch-mock file.

---

ui/app/util/object-assign.ts, line 7 [r2] (raw file):

/* tslint:disable */

if (typeof Object.assign != 'function') {

Might want to mention where you got the polyfill from. I'm guessing here? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign#Polyfill

---

Comments from Reviewable

[tamird]

Indeed, arrow functions are a bad idea with mocha: mochajs/mocha#1969. PTAL.

---

Review status: 0 of 15 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

---

ui/app/containers/events.spec.tsx, line 30 [r2] (raw file):

Previously, maxlang (Max Lang) wrote…

You didn't replace all the describe functions.

Why don't they need a proper closures anymore?

You're right, backed this out.

---

ui/app/redux/metrics.spec.ts, line 224 [r2] (raw file):

Previously, maxlang (Max Lang) wrote…

Here especially, I'm concerned this won't refer to the correct thing.

Backed this out.

---

ui/app/redux/ui.ts, line 31 [r2] (raw file):

Previously, maxlang (Max Lang) wrote…

We should probably stop using Object.assign here like we have in the rest of the code.

New code would be:

state = _.clone(state);
state[payload.key] = payload.value;
return state;

Done.

---

ui/app/util/fetch-mock.ts, line 8 [r2] (raw file):

Previously, maxlang (Max Lang) wrote…

Do we want to distinguish this as a test util?

Meh. Do you feel that we should?

---

ui/app/util/object-assign.ts, line 1 [r2] (raw file):

Previously, maxlang (Max Lang) wrote…

We should add a comment at the top explaining why this is necessary, since none of our code will use it directly if we remove it from ui.ts. I'm almost in favor of just adding it to the fetch-mock file.

Good idea, but I actually pulled this out in a .js file and a .d.ts. The TS compiler doesn't like all the implicit `any`s in this polyfill, and I don't really want to deviate from the source.

---

ui/app/util/object-assign.ts, line 7 [r2] (raw file):

Previously, maxlang (Max Lang) wrote…

Might want to mention where you got the polyfill from. I'm guessing here? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign#Polyfill

Yep. Done.

---

Comments from Reviewable

[maxlang]

---

Review status: 0 of 16 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

ui/app/util/fetch-mock.ts, line 8 [r2] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Meh. Do you feel that we should?

Probably fine for now, especially given that it has mock in its name and we don't have any other testutils.

---

Comments from Reviewable

[mrtracy]

---

Review status: 0 of 16 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

Comments from Reviewable