1180955 - Add the ability to override the require function in loaders

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[James Long (:jlongster)]

Many module bundlers like webpack support adding functions which can override various stages of the pipeline. A common use case is plugins that want to do something really special for certain requires.

This would be really useful in our loader as well. For example, this small addition of code allows my to write a powerful BrowserLoader that creates a loader instance that *only* loads modules in it that come from the window I'm working with, and delegates everything else to the standard devtools loader. This is powerful because I can easily reload just my modules by throwing away my BrowserLoader instance, and the devtools loader still caches all the system modules across everything.

```
Here's what it would look like:

function BrowserRequire(baseURI, window) {
  const loaderOptions = devtools.require('@loader/options');

  const opts = {
    id: "jsdebugger",
    paths: loaderOptions.paths,
    sharedGlobal: true,
    sandboxPrototype: window,
    invisibleToDebugger: loaderOptions.invisibleToDebugger,
    overrideRequire: uri => {
      if(!uri.startsWith(baseURI)) {
        return devtools.require(uri);
      }
    }
  };

  const mainLoader = Loader(opts);
  const mainModule = Module(baseURI, baseURI + "main.js");
  return Require(mainLoader, mainModule);
}

// Usage:

let loader = BrowserRequire("resource:///modules/devtools/debugger/", this);
```

I'll attach a patch that implements this. It's quite small!

Comment 1

[James Long (:jlongster)]

small note: above code has a type and it should be `let require = BrowserRequire(...)`

Comment 2

[James Long (:jlongster)]

Attachment: 1180955.patch

I'm totally fine with changing the option name, whatever you want it to be.

Comment 3

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8630210 [details] [diff] [review]
1180955.patch

Review of attachment 8630210 [details] [diff] [review]:
-----------------------------------------------------------------

::: addon-sdk/source/lib/toolkit/loader.js
@@ +580,5 @@
>      let { uri, requirement } = getRequirements(id);
>      let module = null;
> +
> +    if (overrideRequire) {
> +      let exports = overrideRequire(uri);

It's seems a bit odd to me that we pass to `devtools.require` (following your example) the `uri`, instead of the `id`, giving that we want to "override" the require. I was expecting that we forward the id to the other require; also because the `uri` is resolved by using the requirement of one loader/require, and then used by another one… is it made on purpose? In that case, we should be clear why we do that, and adding a comment.

I would prefer, if it's possible, keep those two things separate – using the functionality we already have – to override the `resolve` if we need to alter the uri/id for any reason, and then pass the `id` to the `overrideRequire`, in that case it would be more explicit what's happening.

About the naming, I'm not sure: we use just `opts.resolve` to override the `resolve`, could we just using `opts.require` to override the require? But it's not exactly the same, isn't it?

I was also thinking maybe to exports a `Symbol` from Loader, to pass to `opts.modules` – 'cause basically it's the same of what we do when we set a static object in the `modules` dictionary, it's just "dynamic" – but I'm not totally convinced.

Also, should we have an option to "cache" in `modules` the result of the `overrideRequire`? I mean, in your scenario we delegate the result to another loader/require, so it will be cached on that side, but what if we don't do that? The current `overrideRequire` is pretty generic, and could be anything.

If we want to restrict only to "delegation", maybe we could just limit that functionality to other requires, where we have a pair `predicate` / `require`?

Those are just food for thoughts, we can keep the current approach!

Comment 4

[James Long (:jlongster)]

All good points. I like passing the id instead, but if we do that, I'm not sure how to add a caching option. Because we won't know the fully resolved URI to cache under.

I don't really care how we do this. I'm fine if you want to restrict this functionality specific to implement "require delegation". This way the options would look something like this:

  const opts = {
    id: "browser-loader",
    sharedGlobal: true,
    sandboxPrototype: window,
    paths: Object.assign({}, loaderOptions.paths, dynamicPaths),
    invisibleToDebugger: loaderOptions.invisibleToDebugger,
    shouldDelegate: (id, resolve) => {
      let uri = resolve(id);
      return !uri.startsWith(baseURI) &&
        !uri.startsWith("resource:///modules/devtools/shared/browser");
    }
    delegateRequire: devtools.require
  };

The `shouldDelegate` determines if it should pass the require off to `delegateRequire`. Of course, we can name them something else. Note that I pass a `resolve` function in, because you'll almost always want to test against the full URI.

One problem is that if doing this, you must use (or extend) the same paths as `delegateRequire`. Otherwise the `resolve` here will fail for modules that `delegateRequire` has aliased to something else.

I think this is somewhat a special case, and we shouldn't worry too much about it, because if you are using this you should know what you're doing. I'm really fine with however you want to implement it, but it would be nice to land something this week so I'm not blocked on this.

Comment 5

[Matteo Ferretti [:zer0] [:matteo]]

I tried to do an API that I liked for this purpose, but I wasn't able to find ones, all of them seems add complexity, so I would say that we can stick to the simple one, and maybe improve later, 'cause I don't want to block your work.
I think that something like that would work for your purpose:

const opts = {
    id: "jsdebugger",
    paths: loaderOptions.paths,
    sharedGlobal: true,
    sandboxPrototype: window,
    invisibleToDebugger: loaderOptions.invisibleToDebugger,
    overrideRequire: (id) => {
      let uri = devtools.resolve(id);

      if(!uri.startsWith(baseURI)) {
        return devtools.require(uri);
      }
    }
  };

If you end up with a better name than `overrideRequire` is more than welcome, otherwise we can keep that for the time being. To me the important thing is that we deal with `id` as argument, and maybe we can pass later the resolved `uri` or the `resolve` function as second argument, but since this is not important to your current goal, I prefer avoid to do so until we have a better API or we stabilize that one.

How's it sound?

Comment 6

[James Long (:jlongster)]

Thanks for looking into it! That sounds good. I'm fine if want to call it just `require`, if it's more consistent since have `resolve` which overloads resolving.

Unfortunately that won't quite work though, passing `resolve` as the second argument is important because we can't resolve `id` with the "other" require (here it's the devtools one). If we add any more paths (which I do, just one which will dynamically resolve React to either prod or dev), it won't resolve correctly. This needs to resolve with the current `require`. Does it feel too inelegant to you to pass `resolve` as the second argument?

I'm on vacation so we can take time to figure it out this week, no rush!

Comment 7

[James Long (:jlongster)]

Attachment: 1180955.patch

Includes the changes we talked about: the config option is now just called `require` (still called `overrideRequire` internally for clarity), and it calls the function with the id and resolved URI as first and second arguments.

Comment 8

[James Long (:jlongster)]

Unfortunately I'll still need some help writing tests... I might make a second pass at trying to get the tests running locally so I can do that. This is the last change I need though, would be great to get this all in soon as I've been blocked on these changes for a while (also been on vacation though :))

Comment 9

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8643301 [details] [diff] [review]
1180955.patch

Review of attachment 8643301 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, but yeah, it needs tests. If you could write some of them, I can run for you; or if you have time today we could also try to figure out what's wrong with your jpm / add-on sdk setup. I think we should tests at least a couple of things: 

- Intercepting: using `overrideRequire` to do stuff before the real `require` kick off, and ensure the caching is still working
- Delegate: using `overrideRequire` to do delegate to another loader's `require`, and ensure the caching is still working.

I can also write down this tests, but it has to wait a bit, I can try to do within the week!

Comment 10

[James Long (:jlongster)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #9)
> Comment on attachment 8643301 [details] [diff] [review]
> 1180955.patch
> 
> Review of attachment 8643301 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me, but yeah, it needs tests. If you could write some of them,
> I can run for you; or if you have time today we could also try to figure out
> what's wrong with your jpm / add-on sdk setup. I think we should tests at
> least a couple of things: 
> 
> - Intercepting: using `overrideRequire` to do stuff before the real
> `require` kick off, and ensure the caching is still working

This made me think more about how this works. Currently, the `require` function passed in the config can't manually call the real `require` because it would go into an infinite loop (continually calling into our custom require). The way it works is if you return something, it will use that as the return value from the original `require` call (thus not doing any more work). If you don't return anything, the original `require` will continue normally and process the required id.

Since we are calling the config option just `require` now, is that confusing? It might be a little confusing to see that name and the function not returning anything. Also, you can't do anything *after* the require is processed.

What do you think about this instead? We could pass the real `require` function as the first argument:

  const opts = {
    require: (require, id) => {
      const uri = require.resolve(id);
      if (!uri.startsWith(baseURI) &&
          !uri.startsWith("resource:///modules/devtools/shared/browser")) {
        return devtools.require(uri);
      }
      return require(uri);
    }
  };

This is actually really cool, because now we don't need to pass the uri as an argument! We can just call `require.resolve` ourselves if we need it. You really are overriding `require` this way, and can do anything you want, such as logging the return values of a require.

Comment 11

[James Long (:jlongster)]

Attachment: 1180955.patch

This implements the new behavior described above. Now you get just a `require` function and an `id`, and you can do anything you want. I also added tests, though unfortunately I still can't get tests to run. I did run similar code in a live environment and it worked. Hopefully it works with minimal changes for you.

Comment 12

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to James Long (:jlongster) from comment #10)

> This made me think more about how this works. Currently, the `require`
> function passed in the config can't manually call the real `require` because
> it would go into an infinite loop (continually calling into our custom
> require). The way it works is if you return something, it will use that as
> the return value from the original `require` call (thus not doing any more
> work). If you don't return anything, the original `require` will continue
> normally and process the required id.

I honestly liked this approach. It was plain and simple, and doesn't add anything more that it was strictly necessary. However, I do not have strong opinion about that, it's fine to me also if we pass the original `require` as argument instead of the resolved uri. However, I would like to keep also the behavior where, if you don't return anything, the original `require` is called anyway; and also having `require` replacing `uri` instead of be the first argument. Mostly because a regular require takes `id` as first argument, so I'd like to have a custom one as close as possible. And for simple stuff the original `require` could be not necessary. For example, we could `dump` stuff for debugging purpose:

   require(id) {
     if (id.startsWith("./") {
       dump(`${id}\n`);
     }
   }

And it would be more readable and concise than `require(require, id) {` I think.
But I'm not too picky about this, if you think it would be better be more explicit.

About the review; we sort it out on vidyo for the time being, we still have a issue to solve.

Comment 13

[James Long (:jlongster)]

About switching the argument order, sure! I'll do that.

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #12)
...
> For example, we
> could `dump` stuff for debugging purpose:
> 
>    require(id) {
>      if (id.startsWith("./") {
>        dump(`${id}\n`);
>      }
>    }
> 
> And it would be more readable and concise than `require(require, id) {` I
> think.
> But I'm not too picky about this, if you think it would be better be more
> explicit.

Personally I like the more explicit approach. It's not much more to explicitly do `return require(id)`, and I think it makes it clearer that this replaces the require. If you are debugging something, and the module for some reason accidentally exports `null` or `undefined`, the loader will then load it again because it thought you didn't load anything.

I doubt this feature is going to be used that much, so I don't think this will come up hardly ever anyway. Probably only my browser loader will use it.

> 
> About the review; we sort it out on vidyo for the time being, we still have
> a issue to solve.

Jordan is helping me by running the tests so hopefully I can get them working today.

Comment 14

[James Long (:jlongster)]

Attachment: 1180955.patch

I changed the arguments order, but kept the requirement to return the exported value. If you feel strongly about it, we can change it. I like the explicitness, but let me know.

More excitingly, I got these tests to pass! Jordan was running them on his computer and they are all passing now (at least the loader ones, I assume the rest work). Can you try this patch and see if it works?

Comment 15

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8644625 [details] [diff] [review]
1180955.patch

Review of attachment 8644625 [details] [diff] [review]:
-----------------------------------------------------------------

> I changed the arguments order, but kept the requirement to return the exported value. If you feel strongly about it, we can change it. I like the explicitness, but let me know.

I like both approach, so that why I don't have strong feeling about one option or another; so the patch looks good to me. And all test are passing too! I'm placing r+ as well.
Just one thing: what was the issue with passing a non existing file? I guess you resolved putting `paths`, but why that was happening?

Comment 16

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/f31bd08b796d68405121c112de55d8a4098dbd35
Bug 1180955 - Add the ability to override the require function in loaders

https://github.com/mozilla/addon-sdk/commit/5e534f1ca4f5e060c086e347933b5f4df1c7a2ee
Merge pull request #2029 from ZER0/override-require/1180955

fix Bug 1180955 - Add the ability to override the require function in loaders;  r=@ZER0

Comment 17

[James Long (:jlongster)]

Let's leave this open until it lands on m-c, which I'm going to do soon (with a separate patch here)

Comment 18

[James Long (:jlongster)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c32d4df5400

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/02529a6a4fd7

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/02529a6a4fd7