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

Clean build process so there are no more errors with missing __ function #77

Merged
merged 2 commits into from
Jul 14, 2017

Conversation

karelhala
Copy link
Contributor

This PR fixes errors which were shown in console if any phase of build was ran (npm run build or npm start). Helper function was introduced, which checks if __ exists on window object and if it does not exists it will return string which was passed to it, if it exists it will return result of __(translateString).

No UI changes.

@karelhala karelhala requested review from romanblanco and himdel June 22, 2017 15:10
@karelhala
Copy link
Contributor Author

@romanblanco I was unable to find where dialog editor is used in SUI so I am unable to test this directly in there. Could you try it if it does not break anything?

@romanblanco
Copy link
Member

@karelhala it is not used in SUI anymore. It was removed in PR ManageIQ/manageiq-ui-service#688.

Right now, there is just PR adding it into Ops UI: ManageIQ/manageiq-ui-classic#1398, otherwise it's not used anywhere right now

Copy link
Member

@romanblanco romanblanco left a comment

Choose a reason for hiding this comment

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

tested, works for me 👍 no more errors with missing __ function 🎈 🎆

@@ -9,7 +10,7 @@ class DialogField {
options: any = {}) {
this.icon = icon;
this.label = label;
this.placeholders = Object.assign({
this.placeholders = (<any>Object).assign({
Copy link
Contributor

Choose a reason for hiding this comment

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

This one sounds wrong .. any way to teach tslint about es6-shim? Or, to assume all ES6 methods are available?

Copy link
Contributor

Choose a reason for hiding this comment

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

(the method is defined in ./node_modules/typescript/lib/lib.es6.d.ts, just not sure how to make it use that without using target: "es6")

Copy link
Contributor

Choose a reason for hiding this comment

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

... actually, this works (but not sure if quite the right place..)

diff --git a/tsconfig.json b/tsconfig.json
index 3093662..c97da88 100644
--- a/tsconfig.json
+++ b/tsconfig.json
@@ -7,6 +7,7 @@
     "typeRoots" : ["./node_modules/@types"]
   },
   "files": [
+    "node_modules/typescript/lib/lib.es6.d.ts",
     "src/index.ts"
   ],
   "exclude": [

Copy link
Contributor

@himdel himdel Jun 26, 2017

Choose a reason for hiding this comment

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

@karelhala So looks like this is the proper fix:

diff --git a/tsconfig.json b/tsconfig.json
index 3093662..4d3d5e9 100644
--- a/tsconfig.json
+++ b/tsconfig.json
@@ -4,7 +4,8 @@
     "module": "commonjs",
     "removeComments": false,
     "sourceMap": true,
-    "typeRoots" : ["./node_modules/@types"]
+    "typeRoots" : ["./node_modules/@types"],
+    "lib": ["es2015", "DOM", "ScriptHost"]
   },
   "files": [
     "src/index.ts"

WDYT?

(From microsoft/TypeScript#3429 (comment) )

EDIT: added DOM to prevent new warnings :). (a list)

EDIT2: added ScriptHost too since it's the default when you don't have lib

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, looks much better! 👍

@himdel
Copy link
Contributor

himdel commented Jul 10, 2017

@karelhala I guess you need to rebase..


I'm starting to wonder whether we need a separate repo (autogenerated on change) for dist/ so that we don't have to keep the built version here and have conflicts all the time.

EDIT: done in #83, to a separate branch (bower-dev)

@mtho11
Copy link
Contributor

mtho11 commented Jul 10, 2017

@himdel that sounds like a cleaner solution having dist/ autogenerated to another repo.

@himdel himdel merged commit a35a353 into ManageIQ:master Jul 14, 2017
@himdel himdel self-assigned this Jul 14, 2017
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.

4 participants