1050422 - Add-on SDK needs a way to load resources bundled with a packages.

Status: RESOLVED WONTFIX

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

Description

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

In the CFX era we had self.data.url("./relative/style.css") to get a url to a bundled resource and self.data.load("./relative/style.css") to get the content of that file.

With JPM this still going to work, but now that we'll have a real support for third party modules it is likely that packages would wanna bundle some static assets for others to use as well, which is not covered by self. So we need a new
solution that would work as intended.

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I know there will be a lot of bike-shedding around this so I'll ask everyone to try and list pros and cons of your proposal so we can weight them and decide with one that suits best our needs.

Currently I think that solution that is going to work the best for SDK is following:

1. Extend `require` so that it can be used for loading any dependencies which maybe a module or a static asset bundled with a package. With that `/main.js` will be able to get a `url` to a `/data/style.css` via `require("./data/style.css").url`. Note that require path in this case is relative to the `main.js`. Require for any other file but `.js` or `.json` should return an instance of a `Resource` class which will
have following interface:

Resource.url: String,
Resource.read() -> Promise <String>

Assets from dependencies should work same as modules from dependencies, which is:

`require("dependency/resource/template.html")`.

2. All our APIs should be updated so they could take `Resource` instances where we provide `self.data.url(...)`.


## Pros

1. Users already know how path resolution logic works so asset loading should come natural.
2. We don't encourage blocking IO nor we do it while loading modules, each API can decide to use IO for a given resource based on it's own constraints.
3. It's very easy to make node package that would enable this API for node.
4. It's very easy to make borwserify preprocessor to enable same API for browserify.
5. We can support this long term, even in ES6 modules.

## Cons

1. This is not an established pattern for nodejs as `fs.readSync(__dirname + "/style.css")` is more of thing there, but unfortunately that's not a great fit for SDK as assets in our case aren't in files, neither it's going to be a great fit for web as resources there aren't on fs either, so I expect diff pattern will have to emerge.
2. By requiring resource you get a handle to it not actually a content which may not be ideal in some cases, although we should be able to update Resource class to better meet user needs.
3. This let's users load resource from their dependencies regardless if dependencies wanted them to be exposed or not. On the other hand same issue applies to private modules so nothing new here.

Comment 3

[Christoph Dorn]

This should really happen via `require.resolve("./module")` as in nodejs and other commonjs systems: http://nodejs.org/api/globals.html#globals_require_resolve

First do a file exists on module ID provided. If none found append `.js`. That way resources can be addressed.

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

+1 For `require.resolve('./path/to/module')`, returning a string of the resourceURI.

Comment 5

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #4)
> +1 For `require.resolve('./path/to/module')`, returning a string of the
> resourceURI.

+1 here to - if we just provide this essential bit, people can always build fancier things on top.

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

I'll work on a prototype for this. Will have to handle local resolutions, dependencies (require.resolve("when/defer")), as well as leverage the loader's paths (require.resolve("sdk/styles/browser.css")), as well as using extensions when given

Comment 7

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Just to be clear though, require.resolve should behave exactly like node in the simple case:

jeff@aer ~> node
> require.resolve('gulp')
'/usr/local/lib/node_modules/gulp/index.js'

...eg the *file path* of the module is returned. It's worth considering if we also want require.cache here.

Comment 8

[Christoph Dorn]

If it returns the full file path there must also be a way to derive the resource URL.

Maybe better if it returns the resource URL which can then in turn be converted to the full filesystem path as the other way around is not possible.

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

Once loaded, the file URI no longer makes sense outside of development for the most part, so yeah resource URI would still make it usable:

PageMod({
  contentStyleFile: require.resolve("dep/style.css")
})

And like Christoph said, can always convert any resource URI into a filepath when in development if needed

Comment 10

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #9)
> Once loaded, the file URI no longer makes sense outside of development for
> the most part, so yeah resource URI would still make it usable:
> 
> PageMod({
>   contentStyleFile: require.resolve("dep/style.css")
> })
> 
> And like Christoph said, can always convert any resource URI into a filepath
> when in development if needed

I like it. Is there a way for resolve() to be smart about if it is being loaded from a directory ( eg i unpacked or when we have native jetpacks ) or packed? As a developer I'd rather not have to think about that.

Comment 11

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

The issue with require.resolve is that there is no straight forward analog to ES6 modules as far as I can tell. That's not to march against require.resolve but rather to say that we need something higher level to recommend to users that will be very simple to port to es6 modules.

Comment 12

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

So my suggestion still would be:

PageMod({
  contentStyleFile: require("dep/style.css")
})


We can update PageMod to handle properly objects returned by require for assets.

Comment 13

[Christoph Dorn]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #12)
> PageMod({
>   contentStyleFile: require("dep/style.css")
> })

This hurts my head. Seems like too much magic.

I don't see a problem with the existence of `require.resolve()` in parallel to any ES6 module stuff. Trying to combine this now is way too premature I think. I can see making use of `require.resolve()` from within ES6 modules.

To me `require.resolve()` is an essential feature that any loader that supports `require()` should support.

Comment 14

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Christoph Dorn from comment #13)
> (In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment
> #12)
> > PageMod({
> >   contentStyleFile: require("dep/style.css")
> > })
> 
> This hurts my head. Seems like too much magic.

I'm on the fence. If you're using a module from npm called 'mystyles' and you know there is a style you want to reference in that package at 'css/style1.css', it seems natural to do:

PageMod({
   contentStyleFile: require.resolve("mystyles/css/style1.css") // this is a resource uri
})

As the developer you know what the contents of those modules are so that's fine. Overloading the behaviour of require to do this as well seems too far.

> To me `require.resolve()` is an essential feature that any loader that
> supports `require()` should support.

I think we should unsurprising to node developers - most JS developers these days have written some node.

Comment 15

[Jordan Santell [:jsantell] [@jsantell]]

way too magical for `require` to return a JS exported object in one scenario, and a URL string in another

Comment 16

[Jordan Santell [:jsantell] [@jsantell]]

Bug 1056283 adds `require.resolve`, returning a string resourceURI path. That bug does -not- handle cases of non-JS path resolution, as I think that will get a bit trickier, but that can be handled in this bug. Right now, extensions are whitelisted (json/js/jsm) and defaults to JS, but we can change a loading rule that says if it ends in dot-whatever, then use that instead of falling back to js.

Comment 17

[Erik Vold [:erikvold] (please needinfo? me)]

I think this is similar to node's require.extensions which is deprecated https://nodejs.org/api/globals.html#globals_require_extensions

I think we should leave this to a third party module.

Feel free to reopen if you think it is necessary.