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

Change webpacker to store plugin javascript in corespoding folder #1919

Merged
merged 2 commits into from Aug 15, 2017

Conversation

karelhala
Copy link
Contributor

@karelhala karelhala commented Aug 15, 2017

when webpacker loads files it will store them in plugin's folder in public/packs folder.

Before

Folder structure of public packs was

public/packs/hello_angular.js
public/packs/application.js
public/packs/manifest.json

Usage in haml file:

+%hello-angular
+  Loading...
+= javascript_pack_tag 'hello_angular'

After

Folder structure of public packs

public/packs/manageiq-ui-classic/hello_angular.js
public/packs/manageiq-ui-classic/application.js
public/packs/manifest.json

Usage in haml file:

+%hello-angular
+  Loading...
+= javascript_pack_tag 'manageiq-ui-classic/hello_angular'

Fixes plugin's javascripts

If we were to add another plugin with app/javascript/packs/example.js the chunk name was broken ../../plugin_example/app/javascript/packs/example, this file was later on not accessible and threw 404. This PR changes it that any JS packs will be copied over to parent manageIq core public/packs folder and will be placed within folder of plugin's name. (for example)

plugin_example/example.js

Usage of such JS is (can be used across whole MiQ app, not just in plugin):

+= javascript_pack_tag 'plugin_example/example'

@karelhala
Copy link
Contributor Author

@miq-bot assign @himdel
@miq-bot add_label bug, enhancement

Can someone else look at it and check if it's ok? perhaps @mtho11 @ohadlevy @h-kataria

when webpacker loads files it will store them in plugin's folder in public/packs folder. application chunk name is exception and HAS to stay in parent folder
@miq-bot
Copy link
Member

miq-bot commented Aug 15, 2017

Checked commits karelhala/manageiq-ui-classic@cad5591~...05003f3 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks fine. 👍

@himdel
Copy link
Contributor

himdel commented Aug 15, 2017

LGTM 👍 pretty much just waiting for feedback if any, and travis :)

@himdel himdel merged commit 353f0d3 into ManageIQ:master Aug 15, 2017
@himdel himdel added this to the Sprint 67 Ending Aug 21, 2017 milestone Aug 15, 2017
@himdel
Copy link
Contributor

himdel commented Aug 15, 2017

Merged :)

Cc @mtho11 @ohadlevy - this will break your dev branches

@mtho11
Copy link
Contributor

mtho11 commented Aug 15, 2017

@himdel ok thanks!

@NickLaMuro
Copy link
Member

NickLaMuro commented Aug 15, 2017

@himdel @karelhala This breaks when installed as a manageiq-ui-classic is installed as a git gem because the directory looks something like manageiq-ui-classic-COMMIT-SHA-HERE when bundler installs, and there is basedir logic in the config/webpack/shared.js, but hard coded paths in the layout file.

This passed when you tested this locally, it most likely was with you using override_gem, which won't have this problem, unless you cloned it to something like manageiq-ui-classic-local. In that case, it probably would have broke.

I am partially bringing this up because this is the second time in the past week where this has happened (bug passed in local testing, but failed in the "git gem" scenario). Can we look at ways of somehow testing this in it's git gem form prior to merging? This will most likely happen again in the future, and it would be nice if it was preventable before affecting others (currently breaking appliance/docker builds based off of master).

@NickLaMuro
Copy link
Member

My above comment should be addressed in #1925

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.

None yet

5 participants