Remove bower.json from root folder and bower components from .gitignore

[scw248]

Fixes #240 (<=== Add issue number here)

#240

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

• PR is descriptively titled 📑 and links the original issue above 🔗
• tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with grunt
• code is in uniquely-named feature branch and has no merge conflicts 📁
• screenshots/GIFs are attached 📎 in case of UI updates
• @mention the original creator of the issue in a comment below for help or for a review

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

@rexagod

Thanks!

[welcome]

Thanks for opening this pull request! Dangerbot will test out your code and reply in a bit with some pointers and requests.
There may be some errors, but don't worry! We're here to help! 👍🎉😄

[scw248]

Hi, @rexagod! Could you let me know if my pull request has been accepted? I saw it hasn't been merged yet, but wasn't sure if there's anything else I need to do.

[rexagod]

Hey @scw248! There's no problem with your PR, I just need to ask another collaborator's review on this since that's is the standard procedure (sorry for this delay!). You can be sure, this is some really nice work, and I'd be more than happy to help you contribute on some next level issues (you can pick issues from the "issues" section based on your preferred language, or maybe checkout other repos of PublicLab as well)! Thanks!

[scw248]

No problem at all @rexagod; thanks for clarifying!

[sashadev-sky]

@scw248 @rexagod Sorry for the delay here! This is great work, but I think @jywarren should merge this one because it has to do with dependencies and he is the maintainer.

[jywarren]

This looks great, but just wanted to check - does MapKnitter itself require this file to exist in order to fetch it using Bower (which we'll stop using soon, but for now we still use over there?)

If this isn't an issue, we can go ahead and merge here but just wanted to check.

[sashadev-sky]

@jywarren @rexagod I don't know from experience. But here is some research:

On the bower site there are steps for dropping bower, and in step 3 (update your major semver version) it explains why we have to introduce a new version with this change:

...because Bower depends solely on git/svn tags for version resolution (it doesn’t care what version is in bower.json, you can remove it).

So it sounds like we can delete it as long as we don't remove it from the bower registry.

Also, most of our dependencies in Mapknitter have a bower.json, but some don't, for ex:

1. leaflet.omnivore: https://github.com/mapbox/leaflet-omnivore
2. leaflet: https://github.com/Leaflet/Leaflet

I don't think it's a bad time to go for this change? But I also think the first thing we should address is moving our stylesheet out of the dist folder, because we will have to delete and rebuild these files. Should I open a PR to fix this first? Is there a reason it's in dist in the 1st place?

[jywarren]

Oh but we don't delete the css file from dist, just the js file, ...i think? In any case, yeah as long as we're confident MapKnitter won't need this bower file, we can remove it and before too long MapKnitter will be deprecating bower entirely anyways. Thanks!

[sashadev-sky]

@jywarren if theres a problem we can quickly revert it.. but I think this should all be done in one PR for the sake of that.

we can salvage from this PR just removing bower_components from the .gitignore file. I can make a commit on top of here to remove the deletion of bower.json and open a separate PR to deprecate Bower.

[sashadev-sky]

@jywarren @rexagod we are now ready to merge this, but I couldn't manage to rebase the users PR because of permission errors.

@scw248 sorry for the delay -- we needed to make some other updates before proceeding here. Would you be still be interested in rebasing here so we can merge this.

@rexagod @jywarren if user doesn't respond in ~a week or so how should we proceed?

[jywarren]

I think we can in this case try pulling the branch and rebasing and opening a new PR but linking back to here so there's continuity? Thank you!!!

…

On Sat, Jul 6, 2019 at 5:45 PM Sasha Boginsky ***@***.***> wrote: @jywarren <https://github.com/jywarren> @rexagod <https://github.com/rexagod> we are now ready to merge this, but I couldn't manage to rebase the users PR because of permission errors. @scw248 <https://github.com/scw248> sorry for the delay -- we needed to make some other updates before proceeding here. Would you be still be interested in rebasing here so we can merge this. @rexagod <https://github.com/rexagod> @jywarren <https://github.com/jywarren> if user doesn't respond in ~a week or so how should we proceed? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#241?email_source=notifications&email_token=AAAF6JZSFUKGHMHSS5SXQR3P6EG5XA5CNFSM4HJ2XSHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZLAXVI#issuecomment-508955605>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAF6JY4YQCM2EYNR65DG3DP6EG5XANCNFSM4HJ2XSHA> .

[scw248]

@sashadev-sky I'd be happy to rebase (I tried git rebase master), but I'm getting an error:

CONFLICT (content): Merge conflict in README.md error: Failed to merge in the changes. Patch failed at 0020 Duplicate Removed (#152)

I checked out to the master branch and ran git pull, but it said I am up to date. Could you let me know if I should be running a different set of commands

[sashadev-sky]

@scw248 A "merge conflict" is very common with rebase and it's good to get practice fixing it! It happens because multiple users can work on the same file at the same time from different branches. If one of those updates is merged into main and conflicts with your update (its on a line by line basis, or also in the case of deleting a file that recently saw updates), you need to manually confirm which changes you want to move forward with.

-- this part is just because its good to keep your local main updated and rebase from there --

1. our primary branch is main not master here. Checkout the main branch, confirm you're on it, and rebase it: $ git pull https://github.com/publiclab/Leaflet.DistortableImage.git main.
2. push your changes locally $ git push origin main -f

-- this is the rebase for this branch --
3. checkout this branch and confirm you're on it.
4. $ git rebase main

this time you will have merge conflicts with bower.json. I think the correct way to resolve it would be:
5. $ git rm bower.json
6. $ git rebase --continue

after rebase is done commit changes and force push:
7. $ git commit -m "Rebase files"
8. $ git push -f origin delete-bower-components

[sashadev-sky]

@scw248 also before doing that I suggest you undo the pull from master in case that causes problems:

1. $ git reflog
2. Find the last action you did before checking out master and copy its corresponding HEAD@{#}
3. $ git reset --hard HEAD@{#} - paste the HEAD there

[scw248]

@sashadev-sky, thanks so much! So I'm most of the way through your instructions, but when I ran git rm bower.json I got this error:
((no branch, rebasing delete-bower-components)) Leaflet.DistortableImage // ♥ git rm bower.json bower.json: needs merge rm 'bower.json'

I assume that means I need to merge the main branch and this branch, but want to make sure in case I might mess anything up. Could you please let me know?

[sashadev-sky]

@scw248 I think this article might explain your problem and it can be fixed with $ git rm --force

[sashadev-sky]

It also mentions this command which I would try if the other one doesn't work: $ git diff --name-only --diff-filter=D -z | xargs -0 git rm --cached

[scw248]

@sashadev-sky git rm --force worked! Thank you very much for the help and the article. I think we're okay to merge now

[sashadev-sky]

@jywarren this is ready!! I'll leave it for you to merge when you're ready because its a big dependency update.

On the bower site it says to also "remove all distribution files from repository" in step 2. Is this necessary and also why? If so I will open a follow up PR for that on merge :) Or @scw248 can do that!

[scw248]

@sashadev-sky I'm happy to help here if needed! Just let me know

[welcome]

Congrats on merging your first pull request! 🙌🎉⚡️
Your code will likely be published to https://mapknitter.org in the next few days.
In the meantime, can you tell us your Twitter handle so we can thank you properly?
Now that you've completed this, you can help someone else take their first step!
See: Public Lab's coding community!

[jywarren]

Great, done! Sorry it took me a while to circle back, and many thanks!

[scw248]

@sashadev-sky and @jywarren Thank you very much!

[sashadev-sky]

@jywarren @scw248 I will publish a version bump for this tomorrow! Locally and to npm, then bump it in mapknitter and fix up the svg code there and the readOnly issue 👍👍

[sashadev-sky]

@jywarren @scw248 It looks like we have one project using this as a bower dependency: https://libraries.io/github/TotalActiveMedia/erfgeoviewer so I will take care to reach out to them tomorrow by opening an issue there before posting any version bumps.
@rexagod

But since we're still below version 1 we don't have to abide to semver yet so I will probably just bump the middle #. But i'll just discuss with them

[sashadev-sky]

they'll probably be fine because they use npm too

[jywarren]

awesome!

…

On Thu, Jul 25, 2019 at 4:43 AM Sasha Boginsky ***@***.***> wrote: @jywarren <https://github.com/jywarren> @scw248 <https://github.com/scw248> It looks like we have one project using this as a bower dependency: https://libraries.io/github/TotalActiveMedia/erfgeoviewer so I will take care to reach out to them tomorrow by opening an issue there before posting any version bumps. @rexagod <https://github.com/rexagod> But since we're still below version 1 we don't have to abide to semver yet so I will probably just bump the middle #. But i'll just discuss with them — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#241?email_source=notifications&email_token=AAAF6J4DH34FB3UGPMLXJJLQBFRSJA5CNFSM4HJ2XSHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2YZPGI#issuecomment-514955161>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAF6J7VWGSVR7EU2N67XPTQBFRSJANCNFSM4HJ2XSHA> .