Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

Remove Bower #1466

Merged
merged 16 commits into from
Sep 15, 2018
Merged

Remove Bower #1466

merged 16 commits into from
Sep 15, 2018

Conversation

donaldwasserman
Copy link
Contributor

@donaldwasserman donaldwasserman commented Jul 27, 2018

Fixes #1465.
See issue for detailed package breakdown.

Notes:

  • I'll be updating this for the next week or so, as it takes a bit of time to do the migrations, update the dependencies and make sure things don't break
  • I've been tempated to use the new auto-import thing, but that seems to be breaking as many things as it helps, we'll see

cc @HospitalRun/core-maintainers

@donaldwasserman donaldwasserman changed the title [WIP] Remove Bower Remove Bower Sep 1, 2018
@MatthewDorner
Copy link
Contributor

npm install
I'm getting this error.

"timekeeper": "^1.0.0",
"webrtc-adapter": "^3.1.3"
}
"name": "hospitalrun"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we still need this file? Kill it?

package.json Outdated
"worker-pouch": "2.1.0"
},
"dependencies": {
"broccoli-funnel": "^2.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why were so many packages moved to dependencies and not in the devDeps? I'd say we deliver our assets which already contain everything bundled; probably it's not worth to explicitly require some packages to be installed afterwards. Correct me if I'm wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH, it's because I can never remember the difference in commands between yarn add and the npm i --save-dev.

I assume the electron stuff is a requirement there for how the electron builds happen. I moved everything up to dependencies minus the electron stuff.

Copy link
Contributor

@stukalin stukalin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a general comment I would:

  • fix all bower mentions in our codebase
  • make sure that for each bower package we have at least one acceptance test for at least one page which uses the package.

@stukalin
Copy link
Contributor

stukalin commented Sep 2, 2018

@MatthewDorner probably the same error we had with our ember-validations. After # commit-ish should go a Git tag, not an npm version or whatever.

Copy link
Contributor

@MatthewDorner MatthewDorner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at what the packages do and I'm satisfied they still work and are decently tested, except there aren't tests for the photo/filesystem stuff. Maybe we can create an issue for that separately.

@donaldwasserman
Copy link
Contributor Author

@stukalin All the tests are covered (with the exception of the barcode one, which requires a configuration setting to be toggled to connect to a printer, which if you have an idea of how to handle that, let me know).

Just updated master, hopefully things are still green 🤞

@MatthewDorner
Copy link
Contributor

Still getting this one when I try to npm install, but if I use yarn it will work.
extraneous

@donaldwasserman
Copy link
Contributor Author

donaldwasserman commented Sep 9, 2018

@MatthewDorner What version of NPM are you using? Mine seems to be downloading fine, but I'm on 5.3

Also, I'm running that locally on my mac, rather than on a VM

@MatthewDorner
Copy link
Contributor

MatthewDorner commented Sep 9, 2018

I'm on node 6.14.4 and npm 3.10.10.

Let me try with node 8.11.4 and npm v5.6.0. Result: same problem. But doesn't list pikaday and typeahead at all in the npm output.

Now node 10.10.0 and npm 6.4.1: it crashes when building leveldown. This may be related to something else, I'm on Ubuntu 18.04 now and I needed to install python, make and g++ due to the leveldown dependency (edit: I never had to do that on Ubuntu 16.04), maybe I did something wrong, although it was building fine for the other npm versions.

@donaldwasserman
Copy link
Contributor Author

donaldwasserman commented Sep 9, 2018 via email

@MatthewDorner
Copy link
Contributor

I think it's this, https://stackoverflow.com/questions/43081427/private-github-repo-as-dependency-is-extraneous-on-npm-install, I'm testing to see if I can fix it.

@MatthewDorner
Copy link
Contributor

MatthewDorner commented Sep 9, 2018

I changed the names in package.json and ember-cli-build to what they are in the package.json of those actual repos (to pikaday-time instead of pikaday and typeahead.js instead of typeahead).
diff

I get farther but still get this error unless I use yarn.
barcode

Could just tell people to use yarn?

@donaldwasserman
Copy link
Contributor Author

donaldwasserman commented Sep 10, 2018

@MatthewDorner That's the issue.

The barcode.js issue you're running into is a problem with an upstream package. I fixed the issue and just pointed to my branch.

@donaldwasserman
Copy link
Contributor Author

@MatthewDorner @stukalin You guys see any other issues?

@MatthewDorner
Copy link
Contributor

nope

@MatthewDorner
Copy link
Contributor

If you commit the last changes you referred to I'll test it again, but I think it'll be fine.

@donaldwasserman
Copy link
Contributor Author

Alright, I think it's all good. Just updated, npm installed, yarn installed, tests passed locally, hopefully they'll pass in travis...

Moving forward here, I think.

@donaldwasserman donaldwasserman merged commit 48db39b into HospitalRun:master Sep 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants