1239060 - Add exports hook to loader options

Status: RESOLVED FIXED

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

Description

[James Long (:jlongster)]

Since we (the devtools team) don't have a build step, to achieve certain features we need to have runtime hooks. We already implemented a `require` hook for the BrowserLoader so that it can load system modules from the devtools loader that already has them cached.

We need another hook: the ability to customize what a module exports. It's pretty simple; if this hook exists we just call it with the final value of `module.exports` after loading a module.

A strong use case: we want hot module reloading for React components, and this requires wrapping each React component with a proxy. The module boundary is a good place to do this: in our exports hook, we will check to see if the exported value is a React class, and if so we will automatically wrap it in a proxy. Otherwise we will return the normal exported object.

I already have hot module reloading working with this tweak and it's super helpful. We can tweak JS and immediately patch the running system to see it live.

Comment 1

[James Long (:jlongster)]

Attachment: 1239060.patch

This also renames the `require` option to `requireHook`, which makes it clearer and and easy to name future options, like the new `exportsHook`.

Comment 2

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

Could you write some short but concrete code example about why we need that? I'm not sure I'm following the use case you described, and I want to be sure there isn't a better way to do so – it seems a bit hacky to me and we also continue to add "hooks" that won't scale well on regular javascript module (`import`); but maybe I don't quite understand the scenario.

Thanks!

Comment 3

[James Long (:jlongster)]

It all comes down to the fact that we don't have a build step. Because of that, we need to do any sort of "post-processing" in these hooks.

This hook is purely for development & debugging. It allows you to instruments modules in any way you like.

I don't see it as hacky; it makes our loader extensible. If you are used to webpack or browserify, our loader feels limited because you have no way to "process" modules (although we don't usually want to do that because it happens at runtime...).

There shouldn't be any scaling problems; all these hooks are only invoked if they exist in the options.

The best example I know of is hot module reloading. This allows you to hit "save" in a JavaScript file and your app see the change and knows how to patch running system. To implement the ability to patch components, you would do something like:

exportsHook: (uri, exports) => {
  if(uri.match(/components/)) {
    return wrapComponent(exports);
  }
  return exports;
}

We've already implemented all of this in bug 1239018 actually, so you can see real code there.

Comment 4

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

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

> I don't see it as hacky; it makes our loader extensible. If you are used to
> webpack or browserify, our loader feels limited because you have no way to
> "process" modules (although we don't usually want to do that because it
> happens at runtime...).

But we have it, right? I mean, we have already the `require`'s override. What I'm trying to understand here is why we need another "hook" for the `exports` when tap in the `require` seems to me enough for everything. So that's why I asked for a concrete example,'cause probably I'm missing the use case where it's not enough. In your short example here:

> exportsHook: (uri, exports) => {
>   if(uri.match(/components/)) {
>     return wrapComponent(exports);
>   }
>   return exports;
> }

You could have currently the same thing using:

  require: (id, require) => {
    let uri = require.resolve(id);
    let exports = require(id);

    if (uri.match(/components/)) {
      return wrapComponent(exports);
    }
    return exports;
  }

Comment 5

[James Long (:jlongster)]

No, that doesn't work because the `require` hook is called for every single `require`, as it should be. It sits above the module cache. We don't want to wrap the module object every single time it's required, it needs to be wrapped only once and replace the object that is stored in the module cache.

I think it's a good extension: `requireHook` (which is what I propose renaming the `require` to) is analogous to a `require` call, and `exportsHook` is analogous to a module being run & finished with an exported object.

Comment 6

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

Sorry for the waiting! As discussed on IRC, I tried other approaches to see if there was an alternative way to obtain the same result. Adding functionality, especially ones that are more for our specific needs than others, to a core component shared across different things – e.g. add-ons) makes me super careful about; especially if it's a behavior not portable in JS `import`. Said that, we have of course to considered our needs, so it's just a matter of finding a balance.

Due the fact that we want to wrap `exports`, and `exports` is the result of `require`, and we've already an override mechanism for that, my preferred way was just reuse it, as I said previously: you can easily handle your own cache having a map for the react component, and that's it. But you'd like avoid that part, so other approach was makes possible that the override could also access to the modules' cache. That approach didn't work well, I didn't like it. Another point was that `require` to me is an action that can be hook, where `exports` is not. But then, I understand that what we really need here are two hooks on two action:

a) when we require a module
b) when we load a module

Therefore, we could have a hook for require and one for module load, the latter should looks like:

   loadModuleHook: (module, require /* optional */) => {
     if(module.uri.match(/components/)) {
       return wrapComponent(module);
     }
     // forced return value just to be explicit,
     // and be clear that the object is replaced on
     // loader side

     return module;
   }

That gives to us more flexibility; for example we / add-ons dev can access to the metadata defined in the module (e.g. https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/ui/toolbar.js#L6-L11), and use it or change it for debugging purpose.

In order to return a different `exports`, we have of course to modify the `module` object itself, `wrapComponent` will have something like `modules.exports = ...`; but also `exports` is an object in most cases, and we could actually inject and modifying it anyway, so I don't think it's a big issue.
Plus, at the time of the hook, the `module` is not frozen, so it's doable.

What do you think? `loadModuleHook` is the first thing pop up to my mind, feel free to find a better one!

Comment 7

[James Long (:jlongster)]

Attachment: 1239060.patch

Thanks a lot for the feedback, I like this better. How does this sound?

Comment 8

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

Comment on attachment 8711117 [details] [diff] [review]
1239060.patch

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

Looks good to me! Maybe we could move the `loadModuleHook` before the `loader.checkCompatibility` – if, for any reason, the hook wants to change that behavior for example, otherwise the metadata changes will happens after.

I'd like to have a unit test too as we have done for the `require`'s hook; but I don't mind if you want to do so in a follow-up bug, and land this one before.

Comment 9

[Pulsebot]

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

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a7049678a4ee