654983 - explicit require with 'package/module/path' does not works.

Status: RESOLVED FIXED

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

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Brian: can you confirm this?

Comment 2

[Brian Warner [:warner :bwarner]]

I'll check. I suspect the "lib/" directory is going to be a problem.. require("A/B/C") probably means finding A/package.json and then reading A/lib/B/C.js . I'm liking the idea of getting rid of "lib/" more and more.

Comment 3

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

I think for 1.0 it would be best to do following:

path = readJSON("A/package.json").directories.lib + "B/C.js"

Post 1.0 I think we'll be better off giving up on packages entirely.

Comment 4

[Alexandre Poirot [:ochameau]]

In fact, there is two issues. First, if we are playing with addon-kit and api-utils packages, the module from api-utils will be used instead of the one from addon-kit. Then, paths with package doesn't work.

# addon-kit/lib/high.js
exports.highLevel = true;

# api-utils/lib/high.js
exports.lowLevel = true;

# addon-kit/tests/test-high.js
exports.testHigh = function(test) {
  let high = require("high");
  // Fails as high is the one from api-utils
  test.assert(high.highLevel, 'require("high") is the one from same package, ie addon-kit');
  // Fails as high is the one from api-utils
  test.assert(!high.lowLevel, 'require("high") is not the one from api-utils'); 
  // Throw exception, module not found
  let low = require("api-utils/high");
  test.assert(high.lowLevel, 'require("api-utils/high") works');
}

This is blocking `timer` move to addon-kit and may be `self` moving too.

Comment 5

[Brian Warner [:warner :bwarner]]

Irakli and I hashed a bunch of this out on IRC today. Here's our current
plan:


* all packages' package.json files can have a .directories property, with a
  .directories.lib . The default is "lib". Packages that want to work on both
  NPM and Jetpack should use package.json:.directories.lib="."

* package.json:.dependencies is used to compute a list of searched packages
  for each starting package.
 * option 1: when code in package A does a search, it will only look in the
             packages listed in A/package.json:dependencies
 * option 2 (myk's preference?): search all packages, not just the ones
                                 in A/package.json:.dependencies
 * packages are searched in alphabetical order
 * the current package is always prepended to the list

* require(A) : (no slash)
 1: find package=A with an A/package.json:.main
 2: search packages for PKG/(.directories.lib)/A.js
* require(A/B)
 1: find package=A, use A(.directories.lib)/B.js
 2: search packages for PKG(.directories.lib)/A/B.js

* compatibility:
 * existing addons that use multiple modules (main.js and helper.js)
   - probably do require("helper")
   - by searching current package first, this will continue to work, but
     is vulnerable to a package named "helper" being added to the path
   - authors can protect against that by using require("./helper") or
     require("mypackagename/helper")
 * existing addons and packages omit package.json:.directories . We can add
   .directories.lib= to stdlib, but not author's existing code. To keep using
   existing libraries (with code in lib/), we need .directories.lib to
   default to "lib"
 * NPM
   - current (May-2011) practice is to omit .directories, put code in
     top-level, use lots of ./relative/paths
   - to use these, we'd want .directories.lib to default to "."
   - for SDK-1.0, we value compatibility with existing Jetpack code over NPM
     code. NPM authors can get both with .directories.lib="." . We could make
     --npm=path=XYZ load those packages with a different default.
   - NPM packages put their per-package dependencies in PKG/node_modules/*
     which would require additional work.
 * CommonJS
   - I think package.json:.directories is still defined, even if NPM isn't
     using it.
   - need more examples of non-NPM CommonJS modules

* future goals
 * avoid searching at all: find a way to have authors write
   require("addon-kit/panel") instead of require("panel")
 * get rid of lib/ directory, so require("addon-kit/panel") really means
   addon-kit/panel.js instead of addon-kit/lib/panel.js
 * avoid using package.json:.directories
 * better/easier NPM compatibility
 * investigate ES.next modules instead-of/in-addition-to CommonJS modules

* development tasks:
 * new search algorithm (including .main)
 * add .directories.lib="lib" to all stdlib packages, for clarity
 * change require() statements in stdlib to relative imports
  - protects us against new packages that sort earlier than addon-kit,
    if we *don't* go with only-search-.dependencies
  - tests need to do require("../lib/foo") or require("mypackage/foo")
  - tests should not be searching for anything

Comment 6

[Brian Warner [:warner :bwarner]]

Incidentally, the code for this is in https://github.com/warner/addon-sdk/tree/652227-directories . The revision I just pushed (33d58) has the updated search code and passes all tests. The next step is to change require() statements to use relative imports (actually the first step is to write a tool that locates all non-relative require statements and computes a good replacement, so I can automate the process).

Irakli: please take a look at that branch and see what you think.

Comment 7

[Brian Warner [:warner :bwarner]]

Attachment: pointer to pull request

Ok, I think this is ready for review. https://github.com/mozilla/addon-sdk/pull/161 contains the pull request: it includes the search-logic changes, and some docs, and some limited tests. It does *not* change all the stdlib require() statements: I'm going to make a separate patch for that.

Let me know what you think! I believe that require("package/module/path") should work now.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #5)
> * package.json:.dependencies is used to compute a list of searched packages
>   for each starting package.
>  * option 1: when code in package A does a search, it will only look in the
>              packages listed in A/package.json:dependencies
>  * option 2 (myk's preference?): search all packages, not just the ones
>                                  in A/package.json:.dependencies

I'm ok with restricting the search to packages in .dependencies when there is a .dependencies.  But I think we should search all packages if .dependencies isn't specified.

Comment 9

[Brian Warner [:warner :bwarner]]

ok, the latest commit on my github 652227-directories branch (https://github.com/warner/addon-sdk/commit/67fc116d70222e2b46c56d8fb55783f293fb59ef) implements this suggestion.. give it a try.

Comment 10

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

Brian I have outlined few issues I noticed, please see pull request for details.

Comment 11

[Heather Arthur [:harth]]

(In reply to comment #5)
>    - authors can protect against that by using require("./helper") or
>      require("mypackagename/helper")

I think `require("./helper")` is more explicit about where the code is coming from than
`require("mypackagename/helper)`.

>    - for SDK-1.0, we value compatibility with existing Jetpack code over NPM
>      code. NPM authors can get both with .directories.lib="." . We could make
>      --npm=path=XYZ load those packages with a different default.

Making node authors add a `directories` wouldn't work. I'd like to use current npm packages like
underscore which don't include that.

>  * get rid of lib/ directory, so require("addon-kit/panel") really means
>    addon-kit/panel.js instead of addon-kit/lib/panel.js
>  * avoid using package.json:.directories

I think this is definitely where it should go. I'm leaning toward api-utils and addon-kit modules being built-in.

It seems that we're trying to make packages fit two different things: the built-in modules and user-developed/npm packages. I don't see why api-utils or addon-kit are packages. They are really collections of modules, not packages which expose a single namespace.

We could just differentiate between the api-utils and addon-kit modules in the UI for the docs.

I think authors should be able to use built-in modules with `require('clipboard')` and use packages like node does, in a straightforward way, with deps like `require('coolpackage')` using .main for the exports, and in-package modules with `require('./othermod')`.

Comment 12

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

(In reply to comment #11)
> (In reply to comment #5)
> >    - authors can protect against that by using require("./helper") or
> >      require("mypackagename/helper")
> 
> I think `require("./helper")` is more explicit about where the code is
> coming from than
> `require("mypackagename/helper)`.
> 

You will be able to use both. Also please not that you need `require("mypackagename/helper)` regardless for loading modules from your dependencies.

> >    - for SDK-1.0, we value compatibility with existing Jetpack code over NPM
> >      code. NPM authors can get both with .directories.lib="." . We could make
> >      --npm=path=XYZ load those packages with a different default.
> 
> Making node authors add a `directories` wouldn't work. I'd like to use
> current npm packages like
> underscore which don't include that.

I'd like to be able to do that as well, but if we have to choose between breaking compatibility with npm packages or jetpack packages I'd choose the first one since:

1. They don't really care supporting jetpack.
2. Doing opposite will require updating all existing libs addons.

Also note that defaulting "lib" to package root want solve compatibility issues with npm packages, since npm bundles dependencies into "node_modules" folder which will not work anyway.

At the moment goal is to make it possible to build cross platform packages by specifying  directories.lib to ".".

> 
> >  * get rid of lib/ directory, so require("addon-kit/panel") really means
> >    addon-kit/panel.js instead of addon-kit/lib/panel.js
> >  * avoid using package.json:.directories
> 
> I think this is definitely where it should go. I'm leaning toward api-utils
> and addon-kit modules being built-in.
> 
> It seems that we're trying to make packages fit two different things: the
> built-in modules and user-developed/npm packages. I don't see why api-utils
> or addon-kit are packages. They are really collections of modules, not
> packages which expose a single namespace.
> 
> We could just differentiate between the api-utils and addon-kit modules in
> the UI for the docs.
> 
> I think authors should be able to use built-in modules with
> `require('clipboard')` and use packages like node does, in a straightforward
> way, with deps like `require('coolpackage')` using .main for the exports,
> and in-package modules with `require('./othermod')`.

Authors still will be able to use `require('clipboard')` if they will decide so. Also `require('coolpackage')` and `require('./othermod')` is going to work as expected. In order to match nodejs behaviors only thing you will need to do is make directories.lib ".", unless your code depends on nodejs specific 'index.js' or 'node_modules'.

Comment 13

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

BTW if we'll implement this https://github.com/mozilla/addon-sdk/pull/161#r31122 we will be compatible with more npm packages that use `main` and don't specify `directories.lib`. Unfortunately `index.js` and `node_modules` will still be an issue :(

Comment 14

[Alexandre Poirot [:ochameau]]

I have some concerns about require being more and more different between cfx run and cfx test. We really have to run test in the same environnement than regular addon execution, or, we may face really bad errors that only occurs on addon run :/

Without this patch, both require may behave almost the same ("magic" vs "non-magic" methods in securable-module). But with such modification we may miss some features in "magic" method.

Then if we try to implement same features set in both methods, we are going to implement the same algorithm in JS and Python that could easily lead to differences.

So I think that both test and run have to use same non-magic method, if it's not already planned?

Comment 15

[Brian Warner [:warner :bwarner]]

I don't think these "magic" modules (["chrome", "self", "parent-loader"]) are a big concern: they're implemented by the loader, and generally behave the same whether the linker knows about the require() statement or not. (the exception being how self.data finds out which package the data should come from, and that should already behave the same way in both places).

But I think there's a closely related concern, which is how the require() statement finds its target .js file, and how that can be different between unit-test code and the main (non-test) addon code. The intent of this linker effort has been to make the decisions early, at link time, and record those decisions in static form inside the XPI, so that reviewers can use this information during their review. Clearly there should be just one place where this is done, to avoid divergence, so doing it in cfx was logical. I wanted to make unit test code subject to this same process, but that made the project too big and I had to cut corners to get it done in time.

After 1.0 is done, I'd like to explore that direction. The code I was working on (still visible in https://github.com/warner/addon-sdk/tree/new-linker or maybe the new-linker-no-harness branch) always built an XPI (even for 'cfx test' and 'cfx run'), and copied all the necessary files into it. It removed the runtime search and strictly used the linker-generated manifest to decide how require() should work. That means all test files needed to be scanned by the manifest, and there'd need to be some alternative approach for dynamic imports like how the unit-test-runner loads each test in turn.

So yeah, making test and run behave the same way is on my mind too :).

Comment 16

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

Attachment: Pointer to Github pull request: https://github.com/warner/addon-sdk/pull/2#

Pointer to Github pull-request

Comment 17

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

Comment on attachment 533633 [details]
Pointer to Github pull request: https://github.com/warner/addon-sdk/pull/2#

Adding pull request with changes replicating linker search logic in the securable-module. This is important in order to be sure that `require('foo/bar')` will return same thing for tests as well.

This is patch on top of brians work and since we want to land this changes together it was making more sense to attach patch here.

Comment 18

[Brian Warner [:warner :bwarner]]

Comment on attachment 533633 [details]
Pointer to Github pull request: https://github.com/warner/addon-sdk/pull/2#

Those look ok, I merged them into my tree already, but didn't notice the r? here. Oops. A few comments are on the pull request.

Comment 19

[Brian Warner [:warner :bwarner]]

This has landed in https://github.com/mozilla/addon-sdk/commit/658fb89ca4d3dc4a8cfb9616ae873e979a7f3955 .

(In reply to comment #15)
> 
> But I think there's a closely related concern, which is how the require()
> statement finds its target .js file, and how that can be different between
> unit-test code and the main (non-test) addon code.

Irakli did a lot of work on this, and the result is that, I think, test files should get very similar behavior to the normal lib/ addon code. Post-1.0 I'd like to move to building an XPI all the time (even for 'cfx run' and 'cfx test'), and putting test files in the XPI when testing (meaning they'd be subject to the manifest scan too).

Comment 20

[Will Bamberg [:wbamberg]]

I had a look at the 'Module Search' guide this bug landed. As far as I can see the thing that more or less has to be fixed are the links, which point to 'devguide' rather than 'dev-guide', so won't find anything.

Apart from that:

- I think it could use an introductory section telling me why I should bother reading this.

- Presumably the reason for including it is to help explain to developers how to use require (yes?), but it reads like it's pointed more at implementors or maintainers of the search logic, than at users.

Here's my suggestion: I'll raise a bug for fixing the links, assign it to me, and if I get the time I'll rework the description to make it more user-centric. Does that sound like a plan?