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
bower doesn't resolve submodules #222
Conversation
Hmm, mixing submodules with package management could be a bit messy. I'd love to hear your thoughts before you dive into the code. Should we advocate using one or the other? Or if you do use submodules, use them only for development purposes -- and keep all dependencies as bower components. |
@desandro, I'm open to hearing suggestions for maintaining a package such as ours. We depend fully on a third party module that during our build phase, we concat it into our bundle. We could truly vendorize and pull in our own copy, but I like the cleanliness of a submodule. I'm also not a user of |
@desandro I'm not 100% sure if this was the best solution, but it works in my case. Do you really see anything wrong with fetching submodules? I don't see how it can hurt anything. |
I'm not 100% sure either. If this gets implemented, bower will be a lot slower due to one or two more git processes to spawn, doing nothing on packages without submodules. |
@satazor "a lot slower"? If you're super concerned, it'd be easy to check for the presence of a Or, let the |
Yes it would be better to check .gitmodules first. Checking for it is a lot cheaper than spawning a process. |
Also, we are fighting against #105 with no luck to fetch a forked repo with an specific hash. it still going to the official to fetch the given hash, check iknite/unforked-repo@6628e14e1d so updating dependencies is a dead end for now |
All of the things are broken. 😦 |
@mattrobenolt tests aren't pssing because after #214 is resolved, tests need to be updated. |
If this is going to land, please test thoroughly before and after to make sure it doesn't slow it down too much. Bower is already way too slow. |
@sindresorhus Sure. This started as a really quick idea to see if it made sense. If it's good, I'll back it with some tests. |
@mattrobenolt I'll revisit this once #217 is done. |
A lot of things were done since this PR was open. A new bower architecture was written and I've though about this. I have to agree with @desandro here. If a package has submodules, then it should include them as dependencies in its bower.json file! Sorry @mattrobenolt! |
Should happen somewhere around: https://github.com/twitter/bower/blob/master/lib/core/package.js#L522
I'll try and get a patch for this today unless someone feels compelled to grab it sooner.
Relevant: getsentry/sentry-javascript#61