joeybaker / remapify

Alias directories for browserify
Other
117 stars 23 forks source link

Configuration like README example doesn't work #23

Open jmm opened 9 years ago

jmm commented 9 years ago

I'm trying to get started with this and struggling a bit to understand how to configure it based on the readme.

The readme leads me to believe I can have a setup like this:

- somedir
  bundler.js
  - client
    entry.js
    - views
      home.js

// entry.js
require('views/home');

// bundler.js
b = browserify('./client/entry.js');

remapify_opts = [
  {
    src: './client/views/**/*.js',
    expose: 'views',
    cwd: __dirname,
  }
];

That doesn't work for me (Ubuntu 12, browserify 6.3.3, remapify 1.3.0). I get:

remapify -  found ./client/views/home.js
remapify -  exposing 4 aliases.
...
Error: Cannot find module 'views/home'

To get it to work I have to do:

{
  src: './**/*.js',
  expose: 'views',
  cwd: './client/views',
}

I feel like the usage example must be wrong based on that behavior and the documentation of cwd:

Specify the 'current working directory' for the glob pattern to start
from and for the `expose` option to replace.

Can you please clarify how the configuration is supposed to work and what exactly the usage example is illustrating?

One way or the other, I think it would be a good idea if the example app directory structure and the usage example correspond.

joeybaker commented 9 years ago

hi @jmm I just updated the glob dep. Can you give the latest remapify a try?

jmm commented 9 years ago

Unfortunately I can't even test it now because of #24.

jmm commented 9 years ago

At least one of a) the plugin or b) the documentation is busted with regards to path resolution. Here is a demo (I had to "downgrade" browserify to 6.x to even get it to run [with 1.4.4]), due to #24). Just clone, then:

> npm install
> node ./bundler.js

The documentation says that the options.expose value will alias to the options.cwd value. So in this demo the app/model/game that src/entry.js requires should map to src/model/game.js, but instead it maps to src/src/model/game.js.

I'm leaning toward the plugin being the busted part and will be sending a pull request re: that -- but that breaks most of the tests, which suggests that the documentation is the busted part. Please clarify how this is supposed to work.

jmm commented 9 years ago

Is options.expose supposed to map to options.cwd or to the directory before a globstar in options.src?

jmm commented 9 years ago

It's probably obvious, but this turned out not to be the case:

but that breaks most of the tests, which suggests that the documentation is the busted part.

I was originally trying a solution that seemed to fix the problem but broke the tests, but ended up making a different change that fixed a definite bug, solved the problem, and didn't break any tests.

baseten commented 9 years ago

I'm having exactly the same issue. There is an inconsistency in the doc, in order to expose things as I need them I have to set the path I want to remove as the cwd in options as per this line from the readme: Specify the 'current working directory' for the glob pattern to start from and for the expose option to replace. But in the Usage example the comment states this will expose '__dirname + /client/views/home.js' as 'views/home.js', which is not the case. Which is the designed behaviour?

joeybaker commented 9 years ago

Hi folks. v2 was just released. Please give that a try and see if that fixes this issue.

gprzybylowicz commented 9 years ago

Hi, i've updated today but it doesn't work. Based on example in docs:

{
  src: './**/*.js',
  expose: 'views',
  cwd: './client/views',
}

only part of path(from cmd )is map as views, not entire path to *js file

taylorhakes commented 9 years ago

This issue still exists in v2

leedorian commented 8 years ago

Yes, still doesn't work per example in doc:

{
    src: './client/views/**/*.js',
    expose: 'views',
    cwd: __dirname,
  }
dhruvio commented 8 years ago

The issue is in line 85 of lib/remapify.js.

After doing some research, I'm proposing the API changes to resolve this problem.

The following is an example of the current options object passed into b.plugin:

[
  {
    src: "folder/**/*.js",
    expose: "foo",
    cwd: __dirname
  }
]

I propose the options object changes to the following:

[
  {
    src: path.join(__dirname, "folder/"),
    expose: "foo",
    extensions: ["js", "json"] // optional, defaults to ["js"]
  }
]

By doing this, remapify could construct the globs internally in a standardised way (e.g. path.join(path.resolve(process.cwd(), pattern.src), "**/*.extension")), and correctly map the expose string to the real file paths by doing the following:

// using ES5 for this module
function getAliasMap (realFilePaths, pattern) {
  return realFilePaths.map(function (filePath) {
    return {
      alias: path.join(pattern.expose, filePath.replace(pattern.src, ""),
      filePath: filePath
    };
  });
}

@joeybaker What do you think? Let me know if you need help with a PR.

joeybaker commented 8 years ago

Sure. Send a PR. We'll have to do a major release, but I'm okay with that!

dhruvio commented 8 years ago

Cool. So, I managed to implement it locally, and I'm going through the tests. I wanted to point out that Remapify would lose some configurability with my proposed changes, specifically when it comes to globbing. The new version would essentially be taking the globbing mechanics into Remapify, and no longer expose them via the browserify plugin options. What I've proposed above will support aliasing the nested contents of directories well (**/*.js), but it won't support aliasing 1) shallow contents of directories (/*.js), and 2) single files (/foo.js).

Over time, it would be possible to support (1) and (2), however, it would likely be done by exposing more options to the user. For example, if the src is a directory (sniffed via fs.statsSync(pattern.src)), deep matching (/**/*.js) could be the default, while specifying options.shallow = true will result in shallow matching (/*.js). If src is a file, we create a single file alias.

This is where my head is at right now. My own needs only require deep matching inside a particular directory, so my proposal is sufficient for me. However, I understand there are other users that may need more. I'm happy to help adding support for the more complex options and matching rules I wrote about above, as they seem the best way to address the majority use-cases for globs. Let me know your thoughts, @joeybaker.

dhruvio commented 8 years ago

Also, I tried pushing my changes to a branch, but I got a 403 error (permission denied).

joeybaker commented 8 years ago

@dhruvio Good points. Why not leave the old API intact so globbing will work, and add yours on as an additional layer? Perhaps:

[
  {
    src: "folder/**/*.js",
    // OR
    dir: path.join(__dirname, "folder/"),
    extensions: ["js", "json"] // optional, defaults to ["js"]
    … 
  }
]

You're likely getting a 403 because you tried to push to my repo. You'll want to create a fork, by clicking the fork button: . More here: https://help.github.com/articles/fork-a-repo/

dhruvio commented 8 years ago

You can see my branch here. Note tests are failing.

That sounds like a good option. However, that still won't solve the existing bug where the example documented in README.md doesn't work?

...
src: "folder/**/*.js", // won't work
...
joeybaker commented 8 years ago

That's okay. We can change the readme :)