Discrepancy in use between require() and rechoir.load()

[silkentrance]

When using require() one can use relative paths, e.g. './foo.bar'. When using rechoir.load() from with the very same context, one must use paths relative to the process' cwd instead.

Since rechoir.load() uses a different require() than the module from which it is being called, this actually makes sense.

As such, passing in relative paths to rechoir.load() should be prevented and the user calling that function must first path.resolve() the filepath before calling rechoir.load().

Consequently, the fix introduced with #7 will not work at all.

Should we deprecate the load function or make it require absolute paths to be passed in instead?

Or perhaps, change the API so that upon

rechoir = require('rechoir')(module);

one must pass in the module for which rechoir is being required for. That way, rechoir could customize itself so that it uses the correct require() function and so on...

[tkellen]

Heya! I'm not 100% sure I follow the line of reasoning here--would you mind submitting a failing test case for this scenario?

[silkentrance]

Well it is not so much of a bug but rather an inconvenience.

Say, you have the following project structure

lib/
   index.js
   node-interpret.js
   forms/
       form.iced.md
   templates/
       main.jade
   util/
       string.coffee

Instead of using require() I'd like to use rechoir.load() instead.

With require and from the lib/ level, I can use relative paths, e.g. require('./forms/form'). And from within forms/form.iced.md I would then be able to require util/string, again using relative paths, e.g. require('../util/string').

With rechoir.load(), however, I would have to use paths relative to the process' cwd, which would for example be that of the parent directory of the lib folder, so instead of '../util/string' I would have to load('./src/util/string').

See also the existing test case for load() and requireFor().

  it('should know coco', function () {
    rechoir.registerFor('./test/fixtures/test.co');
    expect(require('./fixtures/test.co')).to.deep.equal(expected);
  });

Here, require takes a different path to the module than registerFor does, simply because it is not aware of the nesting level or rather the cwd(), which will be adjusted for every module being required.

With the presented API change, this could be streamlined, e.g.

  it('should know coco', function () {
    rechoir.registerFor('./fixtures/test.co');
    expect(require('./fixtures/test.co')).to.deep.equal(expected);
  });

A failing test case for this scenario would be

  it('should load using relative paths', function () {
    expect(rechoir.load('./fixtures/test.co')).to.deep.equal(expected);
  });

[silkentrance]

I have made a temporary branch over at my clone, see 5f87f7e7e8b40a9d36ea76cb3c12830532f12b8f

The branch is actually called failing-on-relative-paths.

[tkellen]

Pretty swamped here today, I will review this stuff first thing tomorrow. Thanks for the detailed examples!

[silkentrance]

@tkellen just use your tools depicted in your avatar ,-) - but then again, you would be swamped in blood and body parts... heck, be careful :laughing:

[tkellen]

+100 to this change!

[tkellen]

rechoir = require('rechoir')(module); that is.

[tkellen]

Upon further reflection, I think it's okay to remove load entirely. I don't need it for the primary use case this module was written for (Liftoff). Can you live without it @silkentrance?

[silkentrance]

Good, my initial design for my own library also only provided a function similar to registerFor and let the user use require() for the rest.

[tkellen]

Brilliant! Closing this.

[phated]

I just pushed the removal of load to master and referenced this issue.

[tkellen]

Roger that. It's still referenced in the README.

[phated]

Missed the usage, good catch