-
Notifications
You must be signed in to change notification settings - Fork 23
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
Comments
hi @jmm I just updated the glob dep. Can you give the latest remapify a try? |
Unfortunately I can't even test it now because of #24. |
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 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. |
Is |
It's probably obvious, but this turned out not to be the case:
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. |
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 |
Hi folks. v2 was just released. Please give that a try and see if that fixes this issue. |
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 |
This issue still exists in v2 |
Yes, still doesn't work per example in doc: {
src: './client/views/**/*.js',
expose: 'views',
cwd: __dirname,
} |
The issue is in line 85 of 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 [
{
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. // 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. |
Sure. Send a PR. We'll have to do a major release, but I'm okay with that! |
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 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 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. |
Also, I tried pushing my changes to a branch, but I got a 403 error (permission denied). |
@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/ |
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
... |
That's okay. We can change the readme :) |
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:
That doesn't work for me (Ubuntu 12, browserify 6.3.3, remapify 1.3.0). I get:
To get it to work I have to do:
I feel like the usage example must be wrong based on that behavior and the documentation of
cwd
: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.
The text was updated successfully, but these errors were encountered: