search

infusionsoft / bower-locker

Command line tool to create a pseudo-bower lock file.
Apache License 2.0
15 stars 8 forks source link

When some packages don't have `_resolution` property, locker will fail. #5

Closed AbdoDabbas closed 7 years ago

AbdoDabbas commented 7 years ago

Hi @MaKleSoft, This is a great tool, but the first time I'm using and I faced an error, I had this in my bower.json file's devDependencies property: "devDependencies": { "sinon": "http://sinonjs.org/releases/sinon-1.12.1.js", .... } So bower-locker failed because that in the .bower.json file of this package there will be no _resolution property. I had to open the source code and do some fixes and did some changes too, please correct me if this is not a good way to go:

  1. Replaced the old way of setting dependency version value to git URL and the release number with setting only the release number as it's enough to be able to install a packages.
  2. Added a check for commit variable as it may not exists for some packages.
  3. Added a check that if release number (extracted from .bower.json of the package) is not valid version number (using semver npm library to check) then set the original value from the old bower.json file we read.
  4. If package exists in both dependencies and devDependencies then fix the release number for both (the old way was to delete devDependencies now we don't).

It's working for me now, don't know if it'll break with some other special cases. Here's my fork: https://github.com/AbdoDabbas/bower-locker

MaKleSoft commented 7 years ago

Hi @AbdoDabbas, thanks for submitting this issue and for sharing your code. I'll take a look at your changes and will see what of it we might carry over to this repo.

KristofMorva commented 7 years ago

I had the same issue, a fast fix: #7 It's working for me with that Pull Request, but later as a full solution, I agree with @AbdoDabbas, version numbers should be used wherever possible, because that's enough for installing.

AbdoDabbas commented 7 years ago

@kmorva yes I already did this in my fork, I'm sorry I'm new to github and don't know how to correctly contribute, so I forked the project and did my fixes, @MaKleSoft if there's any move I should take to merge these fixes , if accepted, let me know.

I did a new fix too and pushed it a few days ago, I added resolutions property for all existing packages, to skip asking to specify the version number when conflict occurs.

AbdoDabbas commented 7 years ago

What we need also that is after locking the file to write pre install script to be executed before installing new packages, so that it'll automate the process of:

MaKleSoft commented 7 years ago

Fixed: https://github.com/infusionsoft/bower-locker/pull/7

MaKleSoft commented 7 years ago

@AbdoDabbas Thanks again for sharing your code! If you want to make it easier for us to integrate your changes, please create a pull request like @kmorva did. See https://help.github.com/articles/creating-a-pull-request/ on how to do this.

shawnlonas commented 7 years ago

Thanks @AbdoDabbas for the feature suggestion. I created a new issue for that: https://github.com/infusionsoft/bower-locker/issues/9