Angular components for the Dialog Editor

[romanblanco]

ManageIQ/manageiq-ui-service#54

[romanblanco]

Still not sure about this one, have to try if it works before removing WIP

[karelhala]

The build is failing, because you are mixing .js files in typescript. You should rewrite any javascript file to typescript.

[mtho11]

@karelhala you beat me to it ;)

Steps:

• Convert all *.js to *.ts
• Move from common.js modules to ES6 modules
• Move syntax from ES5 to ES6/typescript

[himdel]

@karelhala, @mtho11 I don't agree. Is there any technical reason ui-components needs to be typescript-only?

It makes sense to me to put all our shared ui components here, whether they're written in ES5, ES6 or TypeScript.

(I'm not saying this component shouldn't be typescript, if it is, even better .. but I am saying that it should not be a requirement.)

[karelhala]

Well the biggest problem is that we don't have loader for .js files, so they are not compiled from es6 to es5 (which is reason why this particular build is failing). However this can be solved by adding babel loader alongside with eslint rules.

But does it makes sense to mix JS and TS in one project? I'd say more reasonable approach would be not to use all TS's features, but still rewriting every JS file to typescript (you don't have to create interfaces, use types or whatever, just replace IIF with export class and leave the rest as is). To be honest, I'd prefer going either full babel with ES6, or Typescript. But mixing these two together feels, like we could not decide what to use so we used both of them.

I understand that it's easier to take already functioning component and just use it here, it sure is faster than rewriting whole thing over and over again. So what about we put babel loader to webpack and allow using JS files in build process (whether they are written in ES6 or 5) and when such component is introduced we create new issue which states that and when somebody has a bit of spare time he will rewrite such component to typescript. So we have more consistent code base.

[mtho11]

What about not adding the babel loader and just keeping everything *.ts and just replacing the IFFE blocks with export class like @karelhala says above. The typescript compiler should handle both ES5 and ES6 syntax, but this way we are not complicating the webpack build and not totally opening the gate to ES5 whatever. I guess this is my endorsement for what @karelhala said above 🥇

[himdel]

Well, I guess the difference is that I'm thinking of this as a bag for reusable components, not a library with a consistent style and language.. So in my mind, we should support all the options.

That said, @romanblanco seems OK with updating the PR to something typescript-like, so I guess we don't have to have that discussion here and now.

But just to be clear about this, TS is not able to handle ES6 syntax. let foo = {}; foo.bar = 5; .. see if it works :).

[mtho11]

@himdel It does if you play with implicit any stuff. There are compiler settings for this. But you can also just do: let foo:any = {}; foo.bar = 5; and that works.

I just tried it in: http://www.typescriptlang.org/play/

[karelhala]

@romanblanco you do not need to add 3rd party libraries to your index.ts via require just make sure they will be loaded in application which will use your module. If you'd were to require them, they'd become part of our library and we'd have to take care of them (hence the drop in coveralls).

[romanblanco]

@karelhala Thanks, I've just removed them :)

[himdel]

I guess you don't want this .. if I'm reading the docs correctly, this would limit the npm package to just that one file

[himdel]

this.resolveCategories() - resolveCategories doesn't seem to take any arguments

[romanblanco]

@dtaylor113 This PR is now ready, you can do your changes based on this

[karelhala]

@romanblanco found the problem why build started to fail. Looks like Webpack 1 ignored any error and exits every time with 0, even tho some build errors occurred. However webpack 2 is coded better (for us, worse) and does not ignore build errors. However we can force him to ignore these errors, just go to package.json and add webpack || true to prepublish. It's an ugly sollution but it works and we can merge this PR and work on webpack/typescript settings in seperate PR.

...
 "scripts": {
    "start": "webpack --watch",
    "test": "karma start",
    "prepublish": "webpack || true",
    "build": "webpack --config webpack.vendor.config.js && webpack -p",
    "install-vendor": "webpack --config webpack.vendor.config.js",
    "build-docs": "jsdoc -c jsdoc-conf.json"
 },
...

[romanblanco]

Thank you, @karelhala!

[himdel]

Merging, will release a new version together with #58.

The problem with not being able to see the DialogEditor service has something to do with yarn and branches, installing via npm worked just fine.

[mtho11]

@himdel What exactly is the issue with yarn and branches?

[himdel]

@mtho11 no idea about the exact issue, but have a read in ManageIQ/manageiq-ui-service#478 :).

As it is now (with "manageiq-ui-components": "romanblanco/ui-components#dialog_editor" in package.json), it would install a version where dist/js/ui-components.js did not include the substring miqStaticAssets.dialogEditor at all :).

EDIT: possibly fixed in yarn-0.19.1, not sure, 0.16.1 is definitely affected

[himdel]

Released 0.0.12