971098 - Support overrides for toolkit/loader (aka node aliases)

Status: RESOLVED FIXED

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

Description

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

Similar to 'paths' in toolkit/loader, 'aliases' do a string replacement of a module load, except it's done before resolving the path.

Default aliases can be built natively into loader for things like:

`fs` -> `sdk/io/fs` (which would then be resolved via paths for the SDK path)

They can be exposed by the manifest allowing custom overloads, for example, using your own replacement for sdk/tabs:

`aliases: { "sdk/tabs": "./lib/my-tabs" }`

Or instead of a relative file, since this replacement is done before resolving/paths, can use a dependency from npm:

`"aliases": { "fs": "fs-extra" }, "dependencies": { "fs-extra": "*" }`

Comment 1

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

Pinging for feedback/thoughts

Comment 2

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

as discussed today, +1 for aliasing. This needs support from AMO editor tools to flag add-ons that include aliases.

Comment 3

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

+1

Comment 4

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

Weird, my needinfo request should have cleared already..

Comment 5

[Dave Townsend [:mossop]]

Seems like a good idea, I think it's something that could come later though

Comment 6

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

The most immediate need for this is the ability to have a suite of node APIs (outside of the SDK) that allow consumption of all modules from npm:

aliases: {
  'fs': 'sdk/io/fs',
  'http': 'jetpack-http',   (from npm)
  'path': 'jetpack-path',
  ...
}

Comment 7

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

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #6)
> The most immediate need for this is the ability to have a suite of node APIs
> (outside of the SDK) that allow consumption of all modules from npm:
> 
> aliases: {
>   'fs': 'sdk/io/fs',
>   'http': 'jetpack-http',   (from npm)
>   'path': 'jetpack-path',
>   ...
> }

+1, I also don't think it's high priority until we have npm compatibility shipped.


I also wonder if `aliases` should provide deep mapping or shallow one, meaning given above
mapping should `fs/path` resolve to `sdk/io/fs/path` or not ?

Comment 8

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

Irakli, that should resolve correctly, and I'll add a test for that, as these are pre resource-resolving actions.

Comment 9

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

So the issue ran into previously with aliasing..

If we have just straight forward resource mapping, i.e.:

// For changing the "name" of built in SDK modules so node modules can use the SDK fs, this should work!
'fs' => 'sdk/io/fs'

// For overloading and using custom tabs from npm, this should work no problem!
'sdk/tabs' => 'jetpack-custom-tabs'

// For overloading and using custom tabs using a local implementation
'sdk/tabs' => './lib/custom-tabs'

The last example is a bit tricky, a solution would probably be having a fully already resolved path.

After thinking about this more, how important is this, outside of having node modules resolvable to something? Is people overloading modules a practical use case? If I alias "sdk/content/worker", will all the other modules I use use this implementation under the hood, like page-mod? If it does not do this, then what's the point of sharing the namespace? If it *does* do it, then we'll need some mapping of not just "sdk/tabs" -> "resource://addon/.../my-custom-tabs", but the fully resolvable sdk/tabs uri (as SDK modules will reference this relatively)

Comment 10

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

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #9)
> So the issue ran into previously with aliasing..
> 
> If we have just straight forward resource mapping, i.e.:
> 
> // For changing the "name" of built in SDK modules so node modules can use
> the SDK fs, this should work!
> 'fs' => 'sdk/io/fs'
> 
> // For overloading and using custom tabs from npm, this should work no
> problem!
> 'sdk/tabs' => 'jetpack-custom-tabs'
> 
> // For overloading and using custom tabs using a local implementation
> 'sdk/tabs' => './lib/custom-tabs'
> 
> The last example is a bit tricky, a solution would probably be having a
> fully already resolved path.
> 
> After thinking about this more, how important is this, outside of having
> node modules resolvable to something? Is people overloading modules a
> practical use case? If I alias "sdk/content/worker", will all the other
> modules I use use this implementation under the hood, like page-mod? If it
> does not do this, then what's the point of sharing the namespace? If it
> *does* do it, then we'll need some mapping of not just "sdk/tabs" ->
> "resource://addon/.../my-custom-tabs", but the fully resolvable sdk/tabs uri
> (as SDK modules will reference this relatively)


The aliasing should allow sdk/tabs to be aliased imo.  Does this sound good Irakli?

Comment 11

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

I think this is a very import.  We should ship this for these reasons at least:

* This allows us or anyone to ship node parity apis on npm.
* This allows devs to overwrite sdk modules, which they can do for performance reasons (ex: disable addon/window) or for specializations (ex: improve sdk/simple-prefs), and these can be developed on npm.
* This allows devs to provide patches workarounds for npm modules which are too node specific.

Irakli do you agree we should focus work on this bug? and afterwards the bugs for supporting native node apis?

Comment 12

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

Does this sound like a good plan to you Jordan?

Comment 13

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

Jordan if you can't take this one let me know, I'd like to give it a try.

Comment 14

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

(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #10)
> (In reply to Jordan Santell [:jsantell] [@jsantell] from comment #9)
> > So the issue ran into previously with aliasing..
> > 
> > If we have just straight forward resource mapping, i.e.:
> > 
> > // For changing the "name" of built in SDK modules so node modules can use
> > the SDK fs, this should work!
> > 'fs' => 'sdk/io/fs'
> > 
> > // For overloading and using custom tabs from npm, this should work no
> > problem!
> > 'sdk/tabs' => 'jetpack-custom-tabs'
> > 
> > // For overloading and using custom tabs using a local implementation
> > 'sdk/tabs' => './lib/custom-tabs'
> > 
> > The last example is a bit tricky, a solution would probably be having a
> > fully already resolved path.
> > 
> > After thinking about this more, how important is this, outside of having
> > node modules resolvable to something? Is people overloading modules a
> > practical use case? If I alias "sdk/content/worker", will all the other
> > modules I use use this implementation under the hood, like page-mod? If it
> > does not do this, then what's the point of sharing the namespace? If it
> > *does* do it, then we'll need some mapping of not just "sdk/tabs" ->
> > "resource://addon/.../my-custom-tabs", but the fully resolvable sdk/tabs uri
> > (as SDK modules will reference this relatively)
> 
> 
> The aliasing should allow sdk/tabs to be aliased imo.  Does this sound good
> Irakli?

Yes I think so too, actually this makes me think that we should probably call this feature not aliases but rather an overrides. In my mind what this means that each package can have a table that overrides usual module lookup path for only modules with in that package. So when we require module `sdk/tabs` from  package foo we first look if there is `package.json#overrides["sdk/tabs"]` and if it is there we resolve URI to the mapped value instead. I don't think there should be any restrictions on keys users will put into `overrides` other than it should not start with `.` meaning they should not override relative path lookups.

Comment 15

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

Sounds good to me!

Comment 16

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

So I've been thinking about this over the weekend, and I don't think we need to support overrides for every node_module, and we can just implement this for the add-on instead.

I think this is the case is for two reasons:

1. The main use case we wish to support is an add-on wanting to depend on an npm module which uses a nodejs default api.

2. Implementing overrides for every node_module could be implemented as a second stage if we desire this, but I think there are already ways for a module hosted on npm to switch which module it `require`s based on the platform it is being used on, and we doing need to invent a new one and build that in to Fx.

I'd like some feedback, and if we agree then I can implement this, I think implementing overrides for every module in the node_modules folder is overkill however.

The implementation of overrides only for the add-on is simple I think.  When a `require` is made, we check the overrides for the add-on first, if none exists then continue.

Comment 17

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

Main use case I think is to be able to use things on npm that use native node APIs. Ideally, we should just swap out modules for the user ("fs" maps to "sdk/io/fs"), because without that, this is more stuff people have to search around on docs for to figure out how to do it etc., and if we can just do this for the user, that'd be great. Also alerting the user using a node API that we do not have a native equivilent for

Comment 18

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

FWIW, node 0.12 was just released, with a more stabilized API, if we want to use to target those APIs, and try to cover as much as we can.

Comment 19

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

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #17)
> Main use case I think is to be able to use things on npm that use native
> node APIs. Ideally, we should just swap out modules for the user ("fs" maps
> to "sdk/io/fs"), because without that, this is more stuff people have to
> search around on docs for to figure out how to do it etc., and if we can
> just do this for the user, that'd be great. Also alerting the user using a
> node API that we do not have a native equivalent for

I agree with this, in particular aliasing fs & child_process could leverage a ton of existing and interesting 3rd party content on npm. IIUC, other node modules we've implemented is a pretty small subset of the overall api, correct?

Comment 20

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

I ran into a use case that needed this yesterday. David Walsh wants to use zest-creator[1] in a devtools extension, and if we aliased sdk/io/fs to 'fs' this would 'just work'. The workaround is ugly:

1. npm install
2. edit the require statement to try to load 'sdk/io/fs'

https://github.com/canuckistani/jpm-devtools-template/blob/master/node_modules/zest-creator/zestCreator.js#L14-L19

Comment 21

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

I have been saying this for over a year now :p

Comment 22

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

I think this is important because with this issue resolved 90+% of npm modules will be usable with jpm.

Comment 23

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

(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #22)
> I think this is important because with this issue resolved 90+% of npm
> modules will be usable with jpm.

90%? Really?

Comment 24

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

(In reply to Jeff Griffiths (:canuckistani) from comment #20)
> I ran into a use case that needed this yesterday. David Walsh wants to use
> zest-creator[1] in a devtools extension, and if we aliased sdk/io/fs to 'fs'
> this would 'just work'. The workaround is ugly:
> 
> 1. npm install
> 2. edit the require statement to try to load 'sdk/io/fs'
> 
> https://github.com/canuckistani/jpm-devtools-template/blob/master/
> node_modules/zest-creator/zestCreator.js#L14-L19

Please note that there always have being a simple solution to this issues for add-on dependencies. For example if yo want to alias `fs` to `sdk/io/fs` all you need to do is create file in: '@addon/node_modules/fs.js` with a following content: `module.exports = require('sdk/io/fs')`

As a matter of fact that should also cover sub-dependencies for example if my add-on depends on library `fs-promise` which depends on `fs` everything will work as expected because the way node lookup works:

@root/node_modules/fs-promise/index.js will attempt to find `fs` in the following locations:
@root/node_modules/fs-promise/fs.js
@root/node_modules/fs-promise/fs/index.js
@root/node_modules/fs.js
....

And since add-on created entry for last one that in fact loads our shim everything should work as
expected (not sure if it in fact does, as there were some bugs with lookup, but at least that's a way lookup works in node).

Now what I've being suggesting for overrides was basically this, with a difference of not having to create files, but rather encode mapping in the package.json itself. Ideally we would allow sub-packages to define their own overrides, but we can start with supporting just add-on level overrides as that would cover enough cases to be worth it.

Comment 25

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

Honestly, can you imagine anyone doing that in comment#24? How are people even going to be aware of that workaround?

I would have to look at every native lib that some dependency of mine uses, KNOW how to make a shim for it, make a shim for each one, and how would I even know what native node libs we have API-similarity with, and what module that's supposed to map to?

This is such a massive cognitive overhead just to *make things work*, that if we (IMO) either support transparent mapping automatically, or just not even bother to implement.

Comment 26

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

The goal of this is to easily provide access to the node ecosystem to addon developers, with varying experience, so they can more easily create addons without having to reinvent the wheel. Anything we can provide out-of-the-box is a huge win.

Comment 27

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

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #26)
> The goal of this is to easily provide access to the node ecosystem to addon
> developers, with varying experience, so they can more easily create addons
> without having to reinvent the wheel. Anything we can provide out-of-the-box
> is a huge win.

FWIW, I agree, I want the loader to handle this. As a close followup, we need to clearly document what does and does not work, and keep that up to date ( for example, fs.watch* does not currently work, but could once OS.File.watch is in place on all platforms )

Comment 28

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

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #24)
> (In reply to Jeff Griffiths (:canuckistani) from comment #20)
> > I ran into a use case that needed this yesterday. David Walsh wants to use
> > zest-creator[1] in a devtools extension, and if we aliased sdk/io/fs to 'fs'
> > this would 'just work'. The workaround is ugly:
> > 
> > 1. npm install
> > 2. edit the require statement to try to load 'sdk/io/fs'
> > 
> > https://github.com/canuckistani/jpm-devtools-template/blob/master/
> > node_modules/zest-creator/zestCreator.js#L14-L19
> 
> Please note that there always have being a simple solution to this issues
> for add-on dependencies. For example if yo want to alias `fs` to `sdk/io/fs`
> all you need to do is create file in: '@addon/node_modules/fs.js` with a
> following content: `module.exports = require('sdk/io/fs')`
> 
> As a matter of fact that should also cover sub-dependencies for example if
> my add-on depends on library `fs-promise` which depends on `fs` everything
> will work as expected because the way node lookup works:
> 
> @root/node_modules/fs-promise/index.js will attempt to find `fs` in the
> following locations:
> @root/node_modules/fs-promise/fs.js
> @root/node_modules/fs-promise/fs/index.js
> @root/node_modules/fs.js
> ....
> 
> And since add-on created entry for last one that in fact loads our shim
> everything should work as
> expected (not sure if it in fact does, as there were some bugs with lookup,
> but at least that's a way lookup works in node).

This won't actually work atm due to https://github.com/mozilla/addon-sdk/blob/master/lib/toolkit/loader.js#L645

> Now what I've being suggesting for overrides was basically this, with a
> difference of not having to create files, but rather encode mapping in the
> package.json itself. Ideally we would allow sub-packages to define their own
> overrides, but we can start with supporting just add-on level overrides as
> that would cover enough cases to be worth it.

Great this is what I was proposing we do.

Comment 29

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

Attachment: https://github.com/mozilla/addon-sdk/pull/1876

Comment 30

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

Comment on attachment 8570201 [details] [review]
https://github.com/mozilla/addon-sdk/pull/1876

Hey Jordan,

I spoke to Irakli about this on irc yesterday and he was ok with adding the jetpack.overrides key to `package.json` so the rest is just implementation.  He said he was ok with someone else reviewing this patch too, and since he is busy with console work I was hoping you could review this for me?

Note: I made some small changes since your feedback

Comment 31

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

A nice feature to the patch in https://github.com/mozilla/addon-sdk/pull/1876 imo is that we can easily add default overrides in the future, if we want to build certain node modules in to Firefox, I think we have some already covered though.

Comment 32

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

Comment on attachment 8570201 [details] [review]
https://github.com/mozilla/addon-sdk/pull/1876

Looks good! Some comments in there

Comment 33

[[github robot]]

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

https://github.com/mozilla/addon-sdk/commit/bb9bdfa5dca5a0bf41a58ef15e3bc50c943f439c
Bug 971098 Implementing add-on level module overrides

https://github.com/mozilla/addon-sdk/commit/2313e5f1c7bf74a6139fbb3f3a4ab765db204ac2
Merge pull request #1876 from erikvold/971098

Bug 971098 Implementing add-on level module overrides r=jsantell