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

Webpack - clean up loaders, support es6 in node_modules #3506

Merged
merged 5 commits into from Mar 7, 2018
Merged

Webpack - clean up loaders, support es6 in node_modules #3506

merged 5 commits into from Mar 7, 2018

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Mar 1, 2018

Closes #3504

Lot of our webpack config still comes directly unchanged from webpacker.

2 problems: unused loaders for things like ERB & CoffeeScript, and not actually compiling ES6 from node_modules.

Dropping all mentions of coffescript and erb from the code,
unifying the test regexes,
and not excluding node_modules from compilation anymore :).

we have no coffescript or erb code, not in ui-classic, not in ui-componets or in v2v plugin
no point in having the loaders
…al erb

we don't support erb, and the typescript loader was matching anything ending in ts, not .ts
we still want to compile es6 code to es5, regardless whether this comes from our code or from a npm dependency
no coffescript or erb anywhere in manageiq
@himdel
Copy link
Contributor Author

himdel commented Mar 1, 2018

Cc @skateman - we're not really using coffeescript anywhere, are we?

@himdel
Copy link
Contributor Author

himdel commented Mar 1, 2018

Testing:

--- a/app/javascript/packs/application-common.js
+++ b/app/javascript/packs/application-common.js
@@ -13,3 +13,5 @@ window.MiqReact = Object.assign(window.MiqReact || {}, {
   mount: mount,
   componentRegistry: componentRegistry
 });
+
+require('proxy-polyfill');
diff --git a/package.json b/package.json
index 50b4d28fc..273ec531f 100644
--- a/package.json
+++ b/package.json
@@ -34,6 +34,7 @@
     "lodash": "^4.17.4",
     "ng-redux": "^3.5.2",
     "prop-types": "^15.6.0",
+    "proxy-polyfill": "^0.1.7",
     "react": "^16.2.0",
     "react-dom": "^16.2.0",
     "redux-devtools-extension": "^2.13.2",

Run yarn, webpack, and check the compiled manageiq/public/packs/manageiq-ui-classic/application-common.js.

If there's a line which says const unsafeHandler = handler;, this is broken :).
If the line says var unsafeHandler = handler;, this works as intended :).
If there's no such line you're testing it wrong :).

@skateman
Copy link
Member

skateman commented Mar 1, 2018

🚒 🔴 🚒 🔴 ActionCable depends on coffeescript, don't drop it please.

@miq-bot
Copy link
Member

miq-bot commented Mar 1, 2018

Checked commits https://github.com/himdel/manageiq-ui-classic/compare/eb4bfaadbbf244a5c6a4d49a7f5bc95a1d9f1aa7~...248d50913a9f347694f88f196f42b9ddb2b52337 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

@himdel
Copy link
Contributor Author

himdel commented Mar 1, 2018

Aah.. but ActionCable does not go via Webpack right?

So, I can still drop the webpack loader but keep coffee-rails?

@skateman
Copy link
Member

skateman commented Mar 1, 2018

@himdel yes!

@himdel
Copy link
Contributor Author

himdel commented Mar 1, 2018

Thanks, dropped the last commit, should be fine now I hope :)

@skateman
Copy link
Member

skateman commented Mar 1, 2018

I will look into this, but as far as I remember cable wasn't working without coffee-rails included. Of course it is an option to load actioncable the same wat SUI does: through NPM.

@himdel
Copy link
Contributor Author

himdel commented Mar 1, 2018

I will look into this, but as far as I remember cable wasn't working without coffee-rails included. Of course it is an option to load actioncable the same wat SUI does: through NPM.

OK, that ..may actually make sense if we ever go full-NPM :). But I guess we can just keep coffee-rails for now :). Plenty of patternflier fish to fry :)

Copy link
Member

@skateman skateman left a comment

Choose a reason for hiding this comment

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

The Seal of Approval

@mzazrivec mzazrivec self-assigned this Mar 7, 2018
@mzazrivec mzazrivec added this to the Sprint 81 Ending Mar 12, 2018 milestone Mar 7, 2018
@mzazrivec mzazrivec merged commit 16e7222 into ManageIQ:master Mar 7, 2018
@himdel himdel deleted the webpack-3504 branch March 8, 2018 12:38
@himdel himdel mentioned this pull request May 21, 2018
27 tasks
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

4 participants