Skip to content
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

[WIP] Remove bower via bower-away #2571

Closed
wants to merge 2 commits into from
Closed

Conversation

Fryguy
Copy link
Member

@Fryguy Fryguy commented Oct 31, 2017

This migrates the project from bower to yarn via the bower-away tool as recommended by bower themselves. https://bower.io/blog/2017/how-to-migrate-away-from-bower/

@himdel @martinpovolny Please review

TODO: Figure out what to do with the rest of these references to bower

.eslintignore:6:vendor/assets/bower_components/
.eslintrc.json:25:    "Promise": false, // bower: es6-shim
.eslintrc.json:26:    "_": false, // bower: lodash
.eslintrc.json:28:    "angular": false, // bower: angular
.eslintrc.json:29:    "c3": false, // bower: c3
.eslintrc.json:30:    "d3": false, // bower: d3
.eslintrc.json:32:    "moment": false, // bower: moment
.eslintrc.json:33:    "numeral": false, // bower: numeral
.eslintrc.json:34:    "sprintf": false // bower: sprintf
.gitignore:18:vendor/assets/bower_components/
.travis.yml:10:  - vendor/assets/bower_components
.travis.yml:23:- cp bower.json vendor/assets/bower_components
app/assets/javascripts/remote_consoles/spice.js:2://= require spice-html5-bower
config/initializers/assets.rb:1:Rails.application.config.assets.paths << ManageIQ::UI::Classic::Engine.root.join('vendor', 'assets', 'bower_components')
config/initializers/assets.rb:17:  spice-html5-bower
config/initializers/assets.rb:18:  spice-html5-bower/spiceHTML5/spicearraybuffer.js
lib/tasks/manageiq/ui_tasks.rake:2:  task :bower do
lib/tasks/manageiq/ui_tasks.rake:4:      system("bower update --allow-root -F --config.analytics=false") || abort("\n== bower install failed ==")
lib/tasks/manageiq/ui_tasks.rake:18:    Rake::Task['update:bower'].invoke

@ohadlevy
Copy link
Member

thanks @Fryguy looks like a positive change, do you happen to know if spec/manageiq/lib/manageiq/environment.rb is currently used? (it has references to updating bower via bin/update).

@himdel
Copy link
Contributor

himdel commented Oct 31, 2017

@ohadlevy Yes, that code is called during bin/update.

@himdel
Copy link
Contributor

himdel commented Oct 31, 2017

@Fryguy re the TODO:

Keep:

.eslintignore:6:vendor/assets/bower_components/
.gitignore:18:vendor/assets/bower_components/
config/initializers/assets.rb:1:Rails.application.config.assets.paths << ManageIQ::UI::Classic::Engine.root.join('vendor', 'assets', 'bower_components')

.. still using that directory, kept updated by that postinstall

.eslintrc.json:25:    "Promise": false, // bower: es6-shim
.eslintrc.json:26:    "_": false, // bower: lodash
.eslintrc.json:28:    "angular": false, // bower: angular
.eslintrc.json:29:    "c3": false, // bower: c3
.eslintrc.json:30:    "d3": false, // bower: d3
.eslintrc.json:32:    "moment": false, // bower: moment
.eslintrc.json:33:    "numeral": false, // bower: numeral
.eslintrc.json:34:    "sprintf": false // bower: sprintf

.. those are just hints on where those globals come from .. until we actually stop using bower packages, I'd keep them.

app/assets/javascripts/remote_consoles/spice.js:2://= require spice-html5-bower
config/initializers/assets.rb:17:  spice-html5-bower
config/initializers/assets.rb:18:  spice-html5-bower/spiceHTML5/spicearraybuffer.js

.. the package name stays the same

Remove:

lib/tasks/manageiq/ui_tasks.rake:2:  task :bower do
lib/tasks/manageiq/ui_tasks.rake:4:      system("bower update --allow-root -F --config.analytics=false") || abort("\n== bower install failed ==")
lib/tasks/manageiq/ui_tasks.rake:18:    Rake::Task['update:bower'].invoke

..no need to run bower now .. (and I'm not aware of anybody using update:bower directly (nobody should IMO), so no need to keep a compatibility task, I assume).

.travis.yml:10:  - vendor/assets/bower_components
.travis.yml:23:- cp bower.json vendor/assets/bower_components

.. we can just drop the bower-caching on travis I hope, since yarn will cache it for us. Just need to make sure postinstall gets run on travis too.

@himdel
Copy link
Contributor

himdel commented Oct 31, 2017

Overall, yeah, I agree it makes sense to drop bower as a tool now if we can do that without breaking anything :).

(Moving away from bower as a package source and mode of use is still a TODO for H-release (uh.. Hammer time?), though the latter should be relatively easy using webpack ProviderPlugin.)

@himdel
Copy link
Contributor

himdel commented Oct 31, 2017

Oh, one more note...

We're not really married to vendor/assets/bower_components/, maybe we could just change the asset_path line (config/initializers/assets.rb:1) to reference node_modules/@bower_components and we can drop the postinstall task.

@himdel
Copy link
Contributor

himdel commented Oct 31, 2017

error Package "@bower_components/kubernetes-topology-graph@" doesn't have a "version".
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

== yarn failed in /home/himdel/manageiq-ui-classic ==

Looks like if the bower package also has a package.json, it needs to be valid :(.

@miq-bot
Copy link
Member

miq-bot commented Nov 30, 2017

This pull request is not mergeable. Please rebase and repush.

@miq-bot
Copy link
Member

miq-bot commented Dec 11, 2017

This pull request is not mergeable. Please rebase and repush.

@martinpovolny
Copy link
Member

@himdel, @Fryguy : who will finish this PR? Thx!

@Fryguy
Copy link
Member Author

Fryguy commented Jan 9, 2018

I think it's in @himdel's court because I don't know how to move forward

@Fryguy
Copy link
Member Author

Fryguy commented Jan 9, 2018

I'm basically stuck at this step of bower-away:

05:53:55 ~/dev/manageiq-ui-classic (bower-away *) (2.4.2) ✓ bower-away

# Install dependencies with Yarn

Now install dependencies again with:
$ yarn

If you encounter issues during installation, please try:
$ yarn --ignore-engines

If it also fails, you can try following:
$ yarn --ignore-engines --ignore-scripts && yarn postinstall

You can use this command from now on to install both npm and bower dependencies!

Please call bower-away once more when you're done with this!

05:53:58 ~/dev/manageiq-ui-classic (bower-away *) (2.4.2) ✓ yarn
yarn install v1.2.1
[1/5] 🔍  Validating package.json...
[2/5] 🔍  Resolving packages...
error Package "@bower_components/kubernetes-topology-graph@" doesn't have a "version".
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

Apparently kubernetes-topology-graph is not a valid package because it doesn't have a version. But when I look at the latest, I think it does have it, so maybe we just have to upgrade? https://github.com/kubernetes-ui/topology-graph

@miq-bot
Copy link
Member

miq-bot commented Jan 9, 2018

Checked commits Fryguy/manageiq-ui-classic@9ed2cd4~...2eb0e34 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 👍

@miq-bot
Copy link
Member

miq-bot commented Jan 29, 2018

This pull request is not mergeable. Please rebase and repush.

@himdel
Copy link
Contributor

himdel commented Jan 30, 2018

Looking into this, so far..

  • some dependency is trying to install phantomjs and fails miserably (can be skipped with yarn --ignore-optional but..)
  • some dependency is trying to run node-sass and fails
  • yarn is very friendly in that when if fails installing, it doesn't quite bother to tell you why it ran the thing that failed or even which package it came from (EDIT: --verbose almost helps :))
  • npm can't handle the syntax, when it sees package#~2.1.1 it tries to git checkout ~2.1.1 (but that's a non-issue as we don't claim to support npm)
  • something tries to bring in bootstrap@4

Maybe it will make more sense to minimize the bower dependencies to a bare minimum manually and only then use bower-away.

(Another reason to do that is that yarn actually treats these bower dependencies in much the same way as bundler does github branches - slowly :).)

(That said, still trying to use package overrides to achieve the quicker result :).)

@himdel
Copy link
Contributor

himdel commented Jan 30, 2018

Resurrecting ManageIQ/manageiq#12581 in smaller parts, hoping to reduce this to only packages which don't have an equivalent npm version.

@Fryguy
Copy link
Member Author

Fryguy commented Feb 13, 2018

@himdel I'm following your adventures in the other PRs...let me know when you want me to rebase this (unless you just want to take it over in a separate PR)

@miq-bot
Copy link
Member

miq-bot commented Aug 20, 2018

This pull request has been automatically closed because it has not been updated for at least 6 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions!

@Fryguy Fryguy reopened this Sep 17, 2018
@Fryguy Fryguy added the pinned label Sep 17, 2018
@miq-bot
Copy link
Member

miq-bot commented Sep 18, 2018

This pull request is not mergeable. Please rebase and repush.

@Fryguy
Copy link
Member Author

Fryguy commented Oct 8, 2018

Closed via #4735

@Fryguy Fryguy closed this Oct 8, 2018
@Fryguy Fryguy deleted the bower-away branch October 12, 2018 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants