LockFile Feature #1748

Closed
wants to merge 25 commits into
from

Conversation

Projects
None yet
@kodypeterson
Contributor

kodypeterson commented Mar 21, 2015

Based on #1592


  • bower.lock doesn't exist:
    • bower install creates bower.lock file only after all-successful non-production installation
    • bower install --production complains there's no bower.lock and exits
    • bower update acts as bower install
  • bower.lock exists dependenies in bower.json match those on bower.lock
    • bower install installs exact packages from bower.lock
    • bower install --production installs exact packages from bower.lock ignoring devDependencies
    • bower update is no-op
    • if installation would change bower.lock, it fails with error message
  • bower.lock exists and there are only extra dependencies in bower.json relative to bower.lock
    • bower install tries to install bower.lock with only new packages from bower.json added. Installation fails if old bower.lock is not a subset of new bower.lock.
    • bower install --production fails
    • bower update --all re-creates bower.lock
  • bower.lock` exists and dependencies in bower.json changed relative to bower.lock
    • bower install fails, and says you need to bower update first
    • bower install --production fails
    • bower update complains there's no package specified to update
    • bower update --all re-creates bower.lock
    • bower update tries to install bower.lock with version replaced with version from bower.json. Installation fails if old bower.lock is not a subset of new bower.lock (except updated package).

kodypeterson added some commits Mar 21, 2015

Properly install new additions to bower.json wil still locking other …
…dependencies in the existing lock file
@kodypeterson

This comment has been minimized.

Show comment
Hide comment
@kodypeterson

kodypeterson Mar 25, 2015

Contributor

@sheerun - So, when running bower update and nothing has yet to be installed, should a lock file be required as at that point it really acts like bower install?

Contributor

kodypeterson commented Mar 25, 2015

@sheerun - So, when running bower update and nothing has yet to be installed, should a lock file be required as at that point it really acts like bower install?

@kodypeterson

This comment has been minimized.

Show comment
Hide comment
@kodypeterson

kodypeterson Mar 26, 2015

Contributor

@sheerun bump for the question above... 😄

Contributor

kodypeterson commented Mar 26, 2015

@sheerun bump for the question above... 😄

@sheerun

This comment has been minimized.

Show comment
Hide comment
@sheerun

sheerun Mar 27, 2015

Contributor

@kodypeterson Sorry for the delay. Yes, indeed it should do the same as bower install :)

Contributor

sheerun commented Mar 27, 2015

@kodypeterson Sorry for the delay. Yes, indeed it should do the same as bower install :)

@kodypeterson

This comment has been minimized.

Show comment
Hide comment
@kodypeterson

kodypeterson Mar 30, 2015

Contributor

@sheerun any thoughts on the lock file being .bower.lock instead of bower.lock?

Just wondering if we really want to make it a non-dot file...

Contributor

kodypeterson commented Mar 30, 2015

@sheerun any thoughts on the lock file being .bower.lock instead of bower.lock?

Just wondering if we really want to make it a non-dot file...

@kodypeterson

This comment has been minimized.

Show comment
Hide comment
@kodypeterson

kodypeterson Mar 30, 2015

Contributor

Also, I noticed that most of the update tests are invalid as they call tempDir.prepare({JSON}) after the install. Which .prepare will remove all of the contents in the directory.

So, when update runs, it is running against an empty directory with no dependencies installed, so it just acts like bower install 😦

Contributor

kodypeterson commented Mar 30, 2015

Also, I noticed that most of the update tests are invalid as they call tempDir.prepare({JSON}) after the install. Which .prepare will remove all of the contents in the directory.

So, when update runs, it is running against an empty directory with no dependencies installed, so it just acts like bower install 😦

@kodypeterson

This comment has been minimized.

Show comment
Hide comment
@kodypeterson

kodypeterson Mar 30, 2015

Contributor

How should bower link be handled, it in turn calls bower update? Should it follow the same rules as bower update?

Contributor

kodypeterson commented Mar 30, 2015

How should bower link be handled, it in turn calls bower update? Should it follow the same rules as bower update?

@sheerun

This comment has been minimized.

Show comment
Hide comment
@sheerun

sheerun Mar 30, 2015

Contributor

bower link is totally separate thing. It just creates a symlink to some other directory.

I think we don't need to worry about it.

Contributor

sheerun commented Mar 30, 2015

bower link is totally separate thing. It just creates a symlink to some other directory.

I think we don't need to worry about it.

@kodypeterson

This comment has been minimized.

Show comment
Hide comment

@theefer theefer referenced this pull request in guardian/recommendations Mar 31, 2015

Merged

Add client-side recommendations for review #7

@sheerun

This comment has been minimized.

Show comment
Hide comment
@sheerun

sheerun Mar 31, 2015

Contributor

Huh. I'm not sure if it's necessary. Maybe we should check what NPM does..

Contributor

sheerun commented Mar 31, 2015

Huh. I'm not sure if it's necessary. Maybe we should check what NPM does..

@kodypeterson

This comment has been minimized.

Show comment
Hide comment
@kodypeterson

kodypeterson Mar 31, 2015

Contributor

@sheerun - It seems it runs an install https://github.com/npm/npm/blob/master/lib/link.js#L134

For now, I have left it as is. Allowing it to run update and allowing the update to run without allowing it to make changes to the lockfile. Your call on if that is correct.

Contributor

kodypeterson commented Mar 31, 2015

@sheerun - It seems it runs an install https://github.com/npm/npm/blob/master/lib/link.js#L134

For now, I have left it as is. Allowing it to run update and allowing the update to run without allowing it to make changes to the lockfile. Your call on if that is correct.

@sheerun

This comment has been minimized.

Show comment
Hide comment
@sheerun

sheerun Mar 31, 2015

Contributor

I think most reasonable is to just call install, instead of of update.
Install doesn't change lockfile by default.

On Tue, Mar 31, 2015 at 11:20 AM, Kody J. Peterson <notifications@github.com

wrote:

@sheerun https://github.com/sheerun - It seems it runs an install
https://github.com/npm/npm/blob/master/lib/link.js#L134

For now, I have left it as is. Allowing it to run update and allowing the
update to run without allowing it to make changes to the lockfile. Your
call on if that is correct.


Reply to this email directly or view it on GitHub
#1748 (comment).

Contributor

sheerun commented Mar 31, 2015

I think most reasonable is to just call install, instead of of update.
Install doesn't change lockfile by default.

On Tue, Mar 31, 2015 at 11:20 AM, Kody J. Peterson <notifications@github.com

wrote:

@sheerun https://github.com/sheerun - It seems it runs an install
https://github.com/npm/npm/blob/master/lib/link.js#L134

For now, I have left it as is. Allowing it to run update and allowing the
update to run without allowing it to make changes to the lockfile. Your
call on if that is correct.


Reply to this email directly or view it on GitHub
#1748 (comment).

@kodypeterson

This comment has been minimized.

Show comment
Hide comment
@kodypeterson

kodypeterson Mar 31, 2015

Contributor

Alright, I think I have everything covered here 😄

Time for some regression testing? 😄

Not sure why there is a -0.12% decrease in code coverage.... Do I need to add more tests or is this going to be ok?

Contributor

kodypeterson commented Mar 31, 2015

Alright, I think I have everything covered here 😄

Time for some regression testing? 😄

Not sure why there is a -0.12% decrease in code coverage.... Do I need to add more tests or is this going to be ok?

@kodypeterson kodypeterson changed the title from [WIP] LockFile Feature to LockFile Feature Mar 31, 2015

@sheerun

This comment has been minimized.

Show comment
Hide comment
@sheerun

sheerun Mar 31, 2015

Contributor

Yeah, I think we need some more tests and peer review on this. I'll try to review it soon.

Could you squash the commits in meaningful pieces?

What about "if installation would change bower.lock, it fails with error message"? This one is actually pretty important as it ensures that production builds are reproducible. For example it should fail when someone changes the commit behind a tag on GitHub.

Contributor

sheerun commented Mar 31, 2015

Yeah, I think we need some more tests and peer review on this. I'll try to review it soon.

Could you squash the commits in meaningful pieces?

What about "if installation would change bower.lock, it fails with error message"? This one is actually pretty important as it ensures that production builds are reproducible. For example it should fail when someone changes the commit behind a tag on GitHub.

@kodypeterson

This comment has been minimized.

Show comment
Hide comment
@kodypeterson

kodypeterson Mar 31, 2015

Contributor

Ah! That is what that means. I will add that functionality, no problem!

I will also add more tests somehow..... Most of the lockfile logic is tested... I will see what else I can add.

Contributor

kodypeterson commented Mar 31, 2015

Ah! That is what that means. I will add that functionality, no problem!

I will also add more tests somehow..... Most of the lockfile logic is tested... I will see what else I can add.

@satazor

This comment has been minimized.

Show comment
Hide comment
@kodypeterson

This comment has been minimized.

Show comment
Hide comment
@kodypeterson

kodypeterson Mar 31, 2015

Contributor

@satazor At first glance it seems that module is for locking a specific file... That is not really what we are trying to accomplish... Am I wrong in what that module does?

Contributor

kodypeterson commented Mar 31, 2015

@satazor At first glance it seems that module is for locking a specific file... That is not really what we are trying to accomplish... Am I wrong in what that module does?

wrumsby added a commit to wrumsby/bower.github.io that referenced this pull request Nov 14, 2015

@wrumsby wrumsby referenced this pull request in bower/bower.github.io Nov 14, 2015

Merged

Added link to BountySource page #174

@sheerun

This comment has been minimized.

Show comment
Hide comment
@sheerun

sheerun Nov 14, 2015

Contributor

@vincentwoo For now it's only in the README. At the very top.

Contributor

sheerun commented Nov 14, 2015

@vincentwoo For now it's only in the README. At the very top.

@Potherca Potherca referenced this pull request in bower/bower.github.io Nov 15, 2015

Closed

Adds a loud banner that calls out for supporting bower #175

@Potherca

This comment has been minimized.

Show comment
Hide comment

@kswedberg kswedberg referenced this pull request in kswedberg/jquery-smooth-scroll Nov 16, 2015

Closed

Add to bower ? #82

@contolini contolini referenced this pull request in cfpb/capital-framework Nov 17, 2015

Closed

[VZ] Overview #197

9 of 11 tasks complete

@jandudulski jandudulski referenced this pull request in tenex/rails-assets Nov 19, 2015

Closed

Future of rails-assets #291

@sloria sloria referenced this pull request in CenterForOpenScience/osf.io Nov 20, 2015

Closed

[ADM-16] [Feature] Admin webpack #4511

@menor menor referenced this pull request in resmio/elzar Nov 30, 2015

Merged

Remove build #27

@tbrisker tbrisker referenced this pull request in theforeman/foreman Dec 2, 2015

Closed

[WIP] Implement autocomplete UI component in AngularJS #2882

@Utsav2

This comment has been minimized.

Show comment
Hide comment
@Utsav2

Utsav2 Dec 13, 2015

Member

@kodypeterson Great work on this.

As @sheerun mentioned before, is it possible to squash this into meaningful commits and rebase against master? It's hard to review in its current state.

Member

Utsav2 commented Dec 13, 2015

@kodypeterson Great work on this.

As @sheerun mentioned before, is it possible to squash this into meaningful commits and rebase against master? It's hard to review in its current state.

@sheerun

This comment has been minimized.

Show comment
Hide comment
@sheerun

sheerun Dec 13, 2015

Contributor

@Utsav2 Could you do it instead and send another PR?

Contributor

sheerun commented Dec 13, 2015

@Utsav2 Could you do it instead and send another PR?

@Utsav2

This comment has been minimized.

Show comment
Hide comment
Member

Utsav2 commented Dec 14, 2015

@alex88 alex88 referenced this pull request in codemwnci/markdown-editpreview-ng.js Jan 26, 2016

Open

Npm package #3

@iBasit

This comment has been minimized.

Show comment
Hide comment
@iBasit

iBasit Jan 27, 2016

If you need any help with testing, please feel free to message me.

iBasit commented Jan 27, 2016

If you need any help with testing, please feel free to message me.

@Garbee Garbee referenced this pull request in google/material-design-lite Feb 2, 2016

Open

Drop bower support #4063

@mpugach mpugach referenced this pull request in DanielHoffmann/jquery-bigtext Feb 16, 2016

Merged

Add npm support #13

@shyiko

This comment has been minimized.

Show comment
Hide comment
@shyiko

shyiko Mar 13, 2016

For what it's worth shrinkwrap functionality is available using bower-shrinkwrap-resolver. You might find it useful until this PR gets merged in.

shyiko commented Mar 13, 2016

For what it's worth shrinkwrap functionality is available using bower-shrinkwrap-resolver. You might find it useful until this PR gets merged in.

@zship zship referenced this pull request in mutualmobile/lavaca Mar 29, 2016

Closed

Remove bower in favor of npm #156

@sloria sloria referenced this pull request in sloria/cookiecutter-flask Apr 1, 2016

Closed

Remove bower in favor of npm #56

@kodypeterson

This comment has been minimized.

Show comment
Hide comment
@kodypeterson

kodypeterson Aug 22, 2016

Contributor

Alright @sheerun what are we going to do with this one? We missed the birthday celebration of this PR... Soon, it will be 2 years old.

It now has a bunch of conflicts, which I can work to resolve. Are the requirements above still accurate? If so, I can work to get a new PR started. If not, can we get a new issue opened with the correct requirements laid out so I can get to work on it. We need to get this functionality into bower.

It is very obvious the demand for it, and I am here to make it happen! Just let me know what I can do to get this going again!

PS: Sorry for dropping the ball on this PR, it sort of fell of my radar.

Contributor

kodypeterson commented Aug 22, 2016

Alright @sheerun what are we going to do with this one? We missed the birthday celebration of this PR... Soon, it will be 2 years old.

It now has a bunch of conflicts, which I can work to resolve. Are the requirements above still accurate? If so, I can work to get a new PR started. If not, can we get a new issue opened with the correct requirements laid out so I can get to work on it. We need to get this functionality into bower.

It is very obvious the demand for it, and I am here to make it happen! Just let me know what I can do to get this going again!

PS: Sorry for dropping the ball on this PR, it sort of fell of my radar.

@sheerun

This comment has been minimized.

Show comment
Hide comment
@sheerun

sheerun Aug 23, 2016

Contributor

@kodypeterson To release it as quickly as possible we'd need to put it in current 1.x version of bower, and make it backward compatible (opt-in). What sounds reasonable is:

  • Squash all commits into one (it makes rebase easier)
  • Rebase this PR onto master
  • Add --lock flag to bower install to enable this feature when bower.json already exists
  • Add a warning if there isn't a lockfile available, prompting to use --lock flag
  • After bower.lock has been created, keep implemented behavior
  • Do not modify any existing tests and only create new ones (when bower.lock is present)
  • Make sure all current tests pass

If we ever release 2.0, we set --lock to be a default.

As for lock itself, I see it contains few extra keys we don't want, especially they contain absolute paths:

  • endpoint, canonicalDir
  "endpoint": {
    "name": "bower",
    "source": "/Users/sheerun/Source/Bower/bower",
    "target": "1.7.9"
  },
  "canonicalDir": "/Users/sheerun/Source/Bower/bower",

Also, we need to put version of bower it was packaged with to version key, plus additional behavior:

  • If bower minor version in bower.lock is greather than current bower version, refuse to install package, indicating that newer bower version is necessary.

I've also encountered an error when installing local package without bower.json:

Stack trace:
TypeError: Cannot read property 'pkgMeta' of undefined
    at /Users/sheerun/Source/Bower/bower/lib/util/lockFile.js:22:84
    at /Users/sheerun/Source/Bower/bower/node_modules/mout/object/forOwn.js:12:27

I can do more testing if we you manage to fix those. Thank you for being interested in continuing your work.

Contributor

sheerun commented Aug 23, 2016

@kodypeterson To release it as quickly as possible we'd need to put it in current 1.x version of bower, and make it backward compatible (opt-in). What sounds reasonable is:

  • Squash all commits into one (it makes rebase easier)
  • Rebase this PR onto master
  • Add --lock flag to bower install to enable this feature when bower.json already exists
  • Add a warning if there isn't a lockfile available, prompting to use --lock flag
  • After bower.lock has been created, keep implemented behavior
  • Do not modify any existing tests and only create new ones (when bower.lock is present)
  • Make sure all current tests pass

If we ever release 2.0, we set --lock to be a default.

As for lock itself, I see it contains few extra keys we don't want, especially they contain absolute paths:

  • endpoint, canonicalDir
  "endpoint": {
    "name": "bower",
    "source": "/Users/sheerun/Source/Bower/bower",
    "target": "1.7.9"
  },
  "canonicalDir": "/Users/sheerun/Source/Bower/bower",

Also, we need to put version of bower it was packaged with to version key, plus additional behavior:

  • If bower minor version in bower.lock is greather than current bower version, refuse to install package, indicating that newer bower version is necessary.

I've also encountered an error when installing local package without bower.json:

Stack trace:
TypeError: Cannot read property 'pkgMeta' of undefined
    at /Users/sheerun/Source/Bower/bower/lib/util/lockFile.js:22:84
    at /Users/sheerun/Source/Bower/bower/node_modules/mout/object/forOwn.js:12:27

I can do more testing if we you manage to fix those. Thank you for being interested in continuing your work.

@sheerun sheerun referenced this pull request Aug 23, 2016

Closed

[WIP] Shrinkwrap feature #1592

@sheerun

This comment has been minimized.

Show comment
Hide comment
@sheerun

sheerun Aug 23, 2016

Contributor

@kodypeterson Also, then I did "bower install angular-route", only "angular-route" got saved to bower.lock, while it should also contain "angular".

Contributor

sheerun commented Aug 23, 2016

@kodypeterson Also, then I did "bower install angular-route", only "angular-route" got saved to bower.lock, while it should also contain "angular".

@kodypeterson

This comment has been minimized.

Show comment
Hide comment
@kodypeterson

kodypeterson Aug 23, 2016

Contributor

@sheerun I am on it, I will see what I can get through this weekend along with cleanup and such!

Contributor

kodypeterson commented Aug 23, 2016

@sheerun I am on it, I will see what I can get through this weekend along with cleanup and such!

@Utsav2
@kodypeterson

This comment has been minimized.

Show comment
Hide comment
@kodypeterson

kodypeterson Aug 23, 2016

Contributor

@Utsav2 awesome! i will see what I can get from that. I have a feeling that both being so long ago the merge process is going to be a rough one. I might just need to cherry pick manually the changes in and maybe reorganize a bit.

Thanks for the reference though!

Contributor

kodypeterson commented Aug 23, 2016

@Utsav2 awesome! i will see what I can get from that. I have a feeling that both being so long ago the merge process is going to be a rough one. I might just need to cherry pick manually the changes in and maybe reorganize a bit.

Thanks for the reference though!

@mcfilib

This comment has been minimized.

Show comment
Hide comment

mcfilib commented Oct 14, 2016

@kodypeterson need any help?

@sheerun

This comment has been minimized.

Show comment
Hide comment
@sheerun

sheerun Oct 14, 2016

Contributor

Maybe a better route is to fix bower support in https://yarnpkg.com/ that already has file locking

Contributor

sheerun commented Oct 14, 2016

Maybe a better route is to fix bower support in https://yarnpkg.com/ that already has file locking

@mcfilib

This comment has been minimized.

Show comment
Hide comment
@mcfilib

mcfilib Oct 14, 2016

@sheerun I'm not entirely sure how practical that is; we're using PureScript which leverages Bower for dependency management and it looks like support for Bower in Yarn is really just in its infancy e.g. yarnpkg/yarn#896.

Does this mean that you would no longer be willing to consider this patch?

mcfilib commented Oct 14, 2016

@sheerun I'm not entirely sure how practical that is; we're using PureScript which leverages Bower for dependency management and it looks like support for Bower in Yarn is really just in its infancy e.g. yarnpkg/yarn#896.

Does this mean that you would no longer be willing to consider this patch?

@sheerun

This comment has been minimized.

Show comment
Hide comment
@sheerun

sheerun Oct 14, 2016

Contributor

Maybe we could at least adopt lockfile format of yarn?

Contributor

sheerun commented Oct 14, 2016

Maybe we could at least adopt lockfile format of yarn?

@joelhandwell

This comment has been minimized.

Show comment
Hide comment
@joelhandwell

joelhandwell Mar 21, 2017

For those who want to use lock feature before this PR merged, bower-locker is a workaround to generate lockfile and commit only bower.json which is newly generated lock file and bower-locker.bower.json which is renamed original bower.json in your repo.

Run following commands to achieve it:

npm install bower-locker -g

or

yarn global add bower-locker

then generate lock file based on existing bower.json file by runing:

bower-locker lock

I want to express big thanks to @shawnlonas for writing this. Guys, let's give it stars if you find it useful https://github.com/infusionsoft/bower-locker

joelhandwell commented Mar 21, 2017

For those who want to use lock feature before this PR merged, bower-locker is a workaround to generate lockfile and commit only bower.json which is newly generated lock file and bower-locker.bower.json which is renamed original bower.json in your repo.

Run following commands to achieve it:

npm install bower-locker -g

or

yarn global add bower-locker

then generate lock file based on existing bower.json file by runing:

bower-locker lock

I want to express big thanks to @shawnlonas for writing this. Guys, let's give it stars if you find it useful https://github.com/infusionsoft/bower-locker

@kael-shipman

This comment has been minimized.

Show comment
Hide comment
@kael-shipman

kael-shipman Apr 12, 2017

Any news on this? Is it stalled out?

Any news on this? Is it stalled out?

@twogee

This comment has been minimized.

Show comment
Hide comment
@twogee

twogee Apr 12, 2017

Why is this PR open when there is #2100?

twogee commented Apr 12, 2017

Why is this PR open when there is #2100?

@sheerun

This comment has been minimized.

Show comment
Hide comment
@sheerun

sheerun Mar 28, 2018

Contributor

Bower is depreacted now and we recommend switching to Yarn that supports version locking and more. Here's guide how to migrate: https://bower.io/blog/2017/how-to-migrate-away-from-bower/

Contributor

sheerun commented Mar 28, 2018

Bower is depreacted now and we recommend switching to Yarn that supports version locking and more. Here's guide how to migrate: https://bower.io/blog/2017/how-to-migrate-away-from-bower/

@sheerun sheerun closed this Mar 28, 2018

@bower bower locked as too heated and limited conversation to collaborators Mar 28, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.