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

matchPrefix in .bowerrc makes npm:name in bower.json not work #17

Open
jakub-g opened this issue Oct 24, 2016 · 6 comments
Open

matchPrefix in .bowerrc makes npm:name in bower.json not work #17

jakub-g opened this issue Oct 24, 2016 · 6 comments

Comments

@jakub-g
Copy link
Contributor

jakub-g commented Oct 24, 2016

.bowerrc

{
    "resolvers": [
        "bower-npm-resolver"
    ],
    "bowerNpmResolver": {
      "matchPrefix": "angular",
      "stripPrefix": false
    }
}

bower.json

{
  "name": "test",
  "dependencies": {
    "angular-translate": "2.11.0",
    "moment": "npm:moment#2.13.0"
  }
}

Run bower install --verbose

Expected:

  • angular-translate is matched due to matchPrefix in .bowerrc
  • moment is matched due to npm:moment syntax in bower.json

Actual:

  • only angular-translate is matched
$ bower install --verbose
bower moment#2.13.0       npm-resolver found matchers: [{"regexp":{}}]
bower moment#2.13.0       npm-resolver checking for: npm:moment
bower moment#2.13.0          ENOTFOUND Package npm:moment not found
@jakub-g
Copy link
Contributor Author

jakub-g commented Oct 24, 2016

What is "expected" behavior in this case is a matter of discussion, but I think it makes sense to assume that npm: syntax should be always matched regardless of the other settings. WDYT @mjeanroy @mhofman ?

@jakub-g
Copy link
Contributor Author

jakub-g commented Oct 24, 2016

In fact I just realized by reading code that this can be turned on with another (undocumented) setting ignoreMatchDefaults:

{
    "resolvers": [
        "bower-npm-resolver"
    ],
    "bowerNpmResolver": {
      "ignoreMatchDefaults": false,
      "matchPrefix": "angular",
      "stripPrefix": false
    }
}

Maybe it should default to false?

@mhofman
Copy link
Contributor

mhofman commented Oct 24, 2016

The problem here would be that the prefix should be stripped for npm: but not for angular, but the stripPrefix config is global, not per-prefix (maybe that should be changed?).

Any reason you're not simply doing bower install --save npm:@angular/angular-core, which should result in your bower as something like "angular-core": "npm:@angular/core#^2.0.0" (or bower install --save npm:angular resulting in "angular": "npm:angular#^1.5.8")

@mjeanroy
Copy link
Owner

mjeanroy commented Nov 7, 2016

Hi @jakub-g,

I see two problems here:

  • The ignoreMatchDefaults is not documented (how it works and what is the default should be documented): easy to fix, I will do that soon.
  • As @mhofman said, the stripPrefix parameter is global. We can add the code to parameterised it per-prefix, but it may add complexity so I'm not sure this is the way to go. We can probably do something simpler: at least, the stripPrefix should strip all prefix declared in the matchPrefix setting. What do you think?

And sorry for the delay, I was in vacation last two weeks ;)

@jakub-g
Copy link
Contributor Author

jakub-g commented Nov 7, 2016

@mhofman @mjeanroy You raised good points about stripPrefix being global.

We can add the code to parameterised it per-prefix, but it may add complexity so I'm not sure this is the way to go.

In fact, why would we not want to strip npm:? Is there any valid use case for that?

To put you in the context, my use case is as follows:

I started using bower-npm-resolver with just matchPrefix: "mycompany", stripPrefix: false but now I also want to migrate external modules to npm (although it's getting tricky, I will open another issue for that :))

this is how it looks like now

matchPrefix: "mycompany", stripPrefix: false, ignoreMatchDefaults: false


{
  "name": "test",
  "dependencies": {
    "mycompany-foobar": "2.11.0",
    "moment": "npm:moment#2.13.0"
  }
}

The same can be achieved without configuring matchPrefix, which would be less complex (although a bit more verbose)

{
  "name": "test",
  "dependencies": {
    "mycompany-foobar": "npm:mycompany-foobar#2.11.0",
    "moment": "npm:moment#2.13.0"
  }
}

If we wanted to cut on complexity, one more option to follow (a bit extreme) could for the resolver to throw an error when matchPrefix is specified, and npm:... entries are present in bower.json, so that you either use only matchPrefix, or only npm:... links. I am not sure if that makes sense, but might be one option to consider.

@jakub-g
Copy link
Contributor Author

jakub-g commented Nov 7, 2016

We can probably do something simpler: at least, the stripPrefix should strip all prefix declared in the matchPrefix setting. What do you think?

sounds good to me

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

3 participants