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

Enhancement/251 migrate from bower to npm #296

Conversation

AllenBW
Copy link
Member

@AllenBW AllenBW commented Oct 28, 2016

☠️ bower, long live bower 👑

Re-configures process for adding dependencies, need to add documentation on this, was thinking of starting a guides folder in where I can explain the process (long story short npm install, add fep location in the stylesheet.html or javascript.html, update test config dependencies to reflect)

Renames a number of gulp tasks to better align with actual function
Removes a number of unnecessary gulp tasks relating to building the test environment

End of the day, not much changed here! Though I would love some 👀 from the following :godmode: :
@chriskacerguis @himdel @jeff-phillips-18 @dtaylor113

closes #251

@AllenBW
Copy link
Member Author

AllenBW commented Oct 28, 2016

@miq-bot add_label enhancement

@@ -5,7 +5,7 @@
'app.core',
'ui.bootstrap',
'patternfly',
'ngSVGAttributes',
'svgBaseFix',
Copy link
Contributor

Choose a reason for hiding this comment

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

Cloned this branch, new svgBaseFix lib is working. LGTM

install:
- yarn install
- bower install -F
- yarn install --force
Copy link
Contributor

Choose a reason for hiding this comment

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

Why --force? Using --force could do very bad things

Copy link
Member Author

Choose a reason for hiding this comment

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

Thats a good question, travis 💩's if its absent, I would say that force isn't so bad, just fetches deps fresh! https://yarnpkg.com/en/docs/cli/install#toc-yarn-install-force

@chriskacerguis chriskacerguis self-assigned this Oct 28, 2016
@chriskacerguis chriskacerguis added this to the Sprint 49 Ending Nov 14, 2016 milestone Oct 28, 2016
@chriskacerguis
Copy link
Contributor

👍 for me, waiting for @himdel and @martinpovolny to take a look.

@@ -83,7 +69,57 @@ module.exports = (function() {
var options = {
files: [].concat(
'./node_modules/phantomjs-polyfill/bind-polyfill.js',
wiredep({devDependencies: true}).js,
'./node_modules/jquery/dist/jquery.js',
Copy link
Contributor

Choose a reason for hiding this comment

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

This is problematic ;). Is there any way to make wiredep work? Is it just that wiredep can't read package.json or pick the right libraries, or is it also that those don't have the necessary info on what to include?

People will notice that adding the lib is not enough and add it to javascripts, but.. these test deps will get out of sync soon, I'm afraid..

Copy link
Member Author

Choose a reason for hiding this comment

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

wiredep is for bower dependencies only ever just bower dependencies, i TOTALLY feel what you are feeling, wait what, more than one place to update deps? I kinda ran outta time and ideas on how to getting those in here, open to suggestions!

ultimately i was going to write up some documentation on how to correctly add dependencies and keep in mind, this is only until we move to angular 2! importing modules will change a great deal of what we presently do...

Copy link
Contributor

Choose a reason for hiding this comment

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

Aaah, too bad :( I was hoping Karma works like Jasmine and just loads a html file.

But oh well, you're right that this is temporary, so maybe people won't break it so much :) Something too keep an eye for when reviewing though ;)

@@ -9,6 +9,6 @@ var router = express.Router();

router.use(express.static('./client'));
router.use(express.static('./.tmp'));
router.use(express.static('./bower_components'));
router.use(express.static('./node_modules'));
Copy link
Contributor

Choose a reason for hiding this comment

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

This will need to be updated in the apache config as well ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

really? our build process hasn't changed, we don't use bower components in the build, we use those bundled concatenated files, this still builds the same way as before.

Copy link
Contributor

Choose a reason for hiding this comment

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

Console templates ;-)

Copy link
Member Author

@AllenBW AllenBW Oct 29, 2016

Choose a reason for hiding this comment

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

oh yeah soooo those are now served from a folder renamed vendor (also pushed to that folder)
but yeah that changed whos da person to beep for that change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Uh, I guess that would be me :D

Ok, I'm seeing they're now copied to public/ui/service/vendor/{no-vnc,spice-html5-bower}.. The tricky part here is that we need to find them on the same url both in devel mode (served by node) and in production mode (served by apache) .. but that can be acomplished by redirecting /ui/service/vendor to /node_modules in node, and assuming the /ui/service/vendor/... URL, I think.

And then, the only change needed outside manageiq-ui-service would be https://github.com/ManageIQ/manageiq/blob/3921e87915b5a69937b9d4a70bb24ab71b97c165/gems/pending/util/miq_apache/miq_apache.rb#L209 .. that bower_components was there for exactly that reason (and only that reason, so you should be able to safely change that to vendor)

Copy link
Member Author

Choose a reason for hiding this comment

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

@himdel acceptable for me to submit a second pr making the above change? or is this a thing fun enough to rope @simaishi into? 👐 🙌

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say feel free to do it, the more people understand these things the better :-) but do tag us there.. :-)

@himdel
Copy link
Contributor

himdel commented Oct 29, 2016

(EDIT: never mind)

When running gulp build, I'm getting

[05:50:39] gulp-inject 1 files into index.html.
[05:50:39] Plumber found unhandled error:
 Error: Error: File not found with singular glob: /home/himdel/manageiq-ui-service/node_modules/bootstrap-select/dist/css/bootstrap-select.css
[05:50:39] Plumber found unhandled error:
 Error: Error: File not found with singular glob: /home/himdel/manageiq-ui-service/node_modules/bootstrap-switch/dist/css/bootstrap3/bootstrap-switch.css
[05:50:39] Plumber found unhandled error:
 Error: Error: File not found with singular glob: /home/himdel/manageiq-ui-service/node_modules/bootstrap-touchspin/src/jquery.bootstrap-touchspin.css
[05:50:39] Plumber found unhandled error:
 Error: Error: File not found with singular glob: /home/himdel/manageiq-ui-service/node_modules/c3/c3.css
[05:50:39] Plumber found unhandled error:
 Error: Error: File not found with singular glob: /home/himdel/manageiq-ui-service/node_modules/google-code-prettify/bin/prettify.min.css
[05:50:39] Plumber found unhandled error:
 Error: Error: File not found with singular glob: /home/himdel/manageiq-ui-service/node_modules/google-code-prettify/bin/prettify.min.js
[05:50:39] Plumber found unhandled error:
 Error: Error: File not found with singular glob: /home/himdel/manageiq-ui-service/node_modules/c3/c3.js
[05:50:39] Plumber found unhandled error:
 Error: Error: File not found with singular glob: /home/himdel/manageiq-ui-service/node_modules/bootstrap-datepicker/dist/js/bootstrap-datepicker.min.js
[05:50:39] Plumber found unhandled error:
 Error: Error: File not found with singular glob: /home/himdel/manageiq-ui-service/node_modules/bootstrap-select/dist/js/bootstrap-select.js
[05:50:39] Plumber found unhandled error:
 Error: Error: File not found with singular glob: /home/himdel/manageiq-ui-service/node_modules/bootstrap-switch/dist/js/bootstrap-switch.js
[05:50:39] Plumber found unhandled error:
 Error: Error: File not found with singular glob: /home/himdel/manageiq-ui-service/node_modules/bootstrap-touchspin/src/jquery.bootstrap-touchspin.js
[05:50:39] Plumber found unhandled error:
 Error: Error: File not found with singular glob: /home/himdel/manageiq-ui-service/node_modules/patternfly-bootstrap-treeview/dist/bootstrap-treeview.min.js

And looks like it's right, those modules' directories are empty, except for a node_modules/jquery..

Either way, gulp build fails .. is it just me?

EDIT: .. that happens only with yarn 0.15, 0,16.1 is fine :)

@himdel
Copy link
Contributor

himdel commented Oct 29, 2016

This looks good, except for that manual test dependency list.. Any chance we could include javascripts.html in the spec html as well instead of that?

@AllenBW
Copy link
Member Author

AllenBW commented Oct 29, 2016

@himdel sooooooo we can't require the file because javascripts.html is a bunch of scripts rather than a bunch of file locations (which is required in the tests)

totally open to suggestions on how to solve this! because it is a mild problem

silver lining, if ever the list starts to deteriorate, tests will fail, chaos will FOLLOW

also thanks for looking <3 super appreciate it.

@Fryguy
Copy link
Member

Fryguy commented Nov 1, 2016

cc @simaishi for build concerns

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.

Migrate bower + npm to only npm for dependency management
5 participants