ForbesLindesay / browserify-middleware

express middleware for browserify, done right
http://browserify.org
MIT License
381 stars 66 forks source link

Don't do the confusing relative-to-current-module thing #21

Closed domenic closed 10 years ago

domenic commented 10 years ago

I know it's kind of nice, but this is the only package that does it, so it gets really confusing to use browserify-middleware alongside other packages. Node's default semantics, across basically everything that accepts a path, is to effectively do path.resolve(givenPath), i.e. resolve relative to the CWD. Breaking this pattern is very weird.

ForbesLindesay commented 10 years ago

I get that it's strange to break the pattern, it is super convenient though. I think you may be right that the consistency is more important than the relative convenience of how it currently works. What would you do about modules though? e.g.

app.get('/js/bundle.js', browserify(['hyperquest', 'concat-stream']));

It would be really painful if you had to specify the full paths for those.

domenic commented 10 years ago

Oh ick, that does kind of suck. Ummm require.resolve('hyperquest') maybe? I dunno, not sure anymore, feel free to close.

gasi commented 10 years ago

I have a project with the following layout:

project
    |--- www
          |--- scripts
                  |--- app.js
          |--- server.js

When I configure browserify-middleware in server.js, I still need to reference app.js as ./www/scripts/app.js instead of ./scripts/app.js.

Is this expected? If not, is there a configuration I have missed?

domenic commented 10 years ago

Yes; you should do path.resolve(__dirname, "./scripts/app.js")

ForbesLindesay commented 10 years ago

Or you can even do:

browserify(require.resolve('./scripts/app.js'));

The require.resolve function resolves the path relative to the current file.

gasi commented 10 years ago

Would you mind explaining what the advantages are of module relative paths?

In my case, it just makes my code far less readable:

Before

browserify([
  {
    './www/scripts/app': {
      expose: 'App'
    },
    './www/node_modules/react/addons': {
      expose: 'react'
    }
  }
], {
  transform: [
    'coffeeify', literalify.configure({
      react: 'window.React'
    })
  ],
  extensions: ['.coffee']
});

After

var modules = [];

var addModule = function(path, options) {
  var module;
  module = {};
  module[require.resolve(path)] = options;
  // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  return modules.push(module);
};

addModule('./scripts/app', {
  expose: 'App'
});

addModule('./node_modules/react/addons', {
  expose: 'react'
});

browserify(modules, {
  transform: [
    'coffeeify', literalify.configure({
      react: 'window.React'
    })
  ],
  extensions: ['.coffee']
});
domenic commented 10 years ago

The advantage is that every time you reference a file, it works the same way as every other node function (e.g. fs.readFile, or third-party modules). The "before" example has extremely unexpected behavior to anyone used to reading normal node code.

gasi commented 10 years ago

@domenic: Thanks for responding and clarifying :smile: I now understand the analogy to fs. However, I still have the issue for that for the browserify([{'module-d': {expose: 'dee'}}][, options]) usage, 'module-d' is a static object key which is cumbersome to generate dynamically, i.e. see after example. Any suggestions besides my after example?

ForbesLindesay commented 10 years ago

One option will be to use ES6 (via traceur or some as yet unreleased version of node) and then do:

browserify([{[require.resolve('module-d')]: {expose: 'dee'}}]);

I realise that doesn't offer a great solution in the short term though.