Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configuration like README example doesn't work #23

Open
jmm opened this issue Dec 23, 2014 · 17 comments
Open

Configuration like README example doesn't work #23

jmm opened this issue Dec 23, 2014 · 17 comments
Assignees

Comments

@jmm
Copy link

jmm commented Dec 23, 2014

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
Copy link
Owner

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

@jmm
Copy link
Author

jmm commented Jan 7, 2015

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

@jmm
Copy link
Author

jmm commented Jan 13, 2015

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
Copy link
Author

jmm commented Jan 13, 2015

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

@jmm
Copy link
Author

jmm commented Jan 13, 2015

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
Copy link

baseten commented Feb 8, 2015

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
Copy link
Owner

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

@gprzybylowicz
Copy link

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
Copy link

This issue still exists in v2

@leedorian
Copy link

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

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

@joeybaker joeybaker self-assigned this Nov 5, 2015
@dhruvio
Copy link

dhruvio commented Dec 12, 2015

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
Copy link
Owner

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

@dhruvio
Copy link

dhruvio commented Dec 12, 2015

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
Copy link

dhruvio commented Dec 12, 2015

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

@joeybaker
Copy link
Owner

@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
Copy link

dhruvio commented Dec 13, 2015

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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants