Make Methods/Stubs Overridable

[acvetkov]

@ilyaigpetrov Hi.

Can you give some examples, when you need to override chrome api methods?

[ilyaigpetrov]

Can you give some examples, when you need to override chrome api methods?

1. Contrived example:
   1. Whenever somebody does chrome.foo.set('bar') you want to also know the date of the last change to foo.
   2. You can't control other extensions that call chrome.foo.set and how they do it.
2. Real example. chrome.proxy.settings.set may be used to set a PAC script (a script that for each request returns what proxy to use if any). Now imagine that you don't set PAC script yourself, but if somebody sets it you want some modifications to be held. E.g. you want to guarantee that only secure proxies are used, or you want proxies returned by PAC script to work only on httpS sites, or maybe you just want to attach modification date as metadata inside the PAC script (for some reason), etc., etc. You don't want to care who sets PAC script and you don't want to control how they set it, you don't want to create tight coupling between PAC modifier and PAC setter. You just want to guarantee some props of the PAC script being set.

[ilyaigpetrov]

You should separate changes that brings new functionality from infrastructure changes (common refactoring)

Ugh, I have to separate all commits into two groups and also split some commits into two commits. Is it possible in git to make such drastic changes to the history? I think I should repeat all changes step by step manually for this. Do you really want me to do it?

[acvetkov]

Do you really want me to do it?

Nope. I need a little time to think about it.

[acvetkov]

@vitalets What do you think about?

[vitalets]

hi @ilyaigpetrov

1. Could you show your test code that requires stubs override?
2. We should definitely check out sinon 2 - is not it implemented there

[ilyaigpetrov]

Here I want this to work:

Mocha.it('is installed (by modifying `chrome.proxy.settings.set`)', function () {

  const originalSet = chrome.proxy.settings.set;
  CachelessRequire('../35-pac-kitchen-api.js');
  Chai.expect(originalSet.notCalled, 'original set not to be called during install').to.be.true;
  Chai.expect(originalSet, 'settings.set to be modified during install').not.to.be.equal(chrome.proxy.settings.set);

});

In the PR I added tests of stub overrides and they pass.

[ilyaigpetrov]

In overriding methods I see a way of loosely coupling my components. PAC modifier (kitchen) component knows nothing about PAC setters. PAC setters know nothing about PAC modifier(s). And they work together. There is chrome.proxy.settings.onChange, but it has shortcomings too:

1. It is triggered by other extensions too, so we have to differentiate changes of my extension from others.
2. I will have to do more operations: catch_event-read-modify-write-ignore_own_event vs intercept-modify-write.

[vitalets]

I get your point. I think it is better not to mutate original chrome object:

  chrome.proxy.settings.set = function(details, cb) {
    const pac = window.utils.getProp(details, 'value.pacScript');
    if (!(pac && pac.data)) {
      return originalSet(details, cb);
    }
    const pacMods = getCurrentConfigs();
    pac.data = pacKitchen.cook( pac.data, pacMods );
    originalSet({value: details.value}, (/* No args. */) => {
      kitchenState(ifIncontinence, null);
      cb && cb();
    });
  };

Instead we can wrap it into own function for setting proxy and use it in other modules:

window.setProxy = function(details, cb) {
    const pac = window.utils.getProp(details, 'value.pacScript');
    if (!(pac && pac.data)) {
      return originalSet(details, cb);
    }
    const pacMods = getCurrentConfigs();
    pac.data = pacKitchen.cook( pac.data, pacMods );
    chrome.proxy.settings.set({value: details.value}, (/* No args. */) => {
      kitchenState(ifIncontinence, null);
      cb && cb();
    });
  };

In that case code is more clean (imho) and no problem with sinon-chrome.

[ilyaigpetrov]

Exposing window.setProxy will work, but components will need to know about existence of each other and you can't delete PAC-modifier without modifying PAC-setter. But yes, I may modify my code. The questions I have:

1. Is modifying API methods a bad practice? Why is it bad?
2. If it is not so bad, should we support it in sinon-chrome? Will sinon-chrome benefit from it?

[vitalets]

Is modifying API methods a bad practice? Why is it bad?

In that case chrome API is system object like all other Javascript objects. Many people consider monkey patching of system objects is bad (but not always). Shortly the problems are listed here in wiki.

but components will need to know about existence of each other and you can't delete PAC-modifier without modifying PAC-setter

Agree. Reducing connectivity is good point. Here you are using chrome.proxy.xxx as some shareable resource that allows PAC-modifier and PAC-setter to be independent. I could suggest to create such shareable resource manually:

// shared.js
exports.setProxy = function(details, cp) { ... }