RA-1121/OMRSJS-5 migrate to webpack #15

pgutkowski · 2016-04-28T08:22:12Z

No description provided.

pgutkowski · 2016-04-28T08:24:41Z

okk, I forgot about tests

pgutkowski · 2016-04-28T09:50:19Z

I'm not able to make bitHound - Code test pass, as it fails on Yeoman placeholders.

psbrandt · 2016-04-28T20:27:58Z

README.md

- - [x] Local deploy with Gulp
- - [x] Package management with [Bower](http:https://bower.io/)
+ - [x] Production build with [Webpack](https://webpack.github.io/)
+ - [x] Local deploy with [Webpack](https://webpack.github.io/)


This is minor, but we probably only need the first Webpack link :).

psbrandt · 2016-04-28T20:46:23Z

Right now manifest.webapp is not included in the local deploy or the production build, so the app is not detected by the OWA module no matter how I try to deploy.

psbrandt · 2016-04-28T20:47:32Z

README.md

 ````

-Be sure to include the following in your `html` files at the position you want the Bower dependencies injected:
+Be sure to include the loading statement in your `.js` files at the modules you want your dependencies injected:


Let's rewrite this sentence as:

To use the installed package, import it as follows:

pgutkowski · 2016-04-29T07:15:01Z

I've fixed packing manifest.webapp. I think if we would treat index.js mainly as entry point, importing all other webapp components there makes sense and is common practice (like eg. here). Anyway, I've moved HTML and CSS imports out of our index.js. Now .css file is separate entry point, but bundled together with .js. I'm waiting with fixing imports example in README and .js template for definite decision.

rkorytkowski · 2016-04-29T11:02:03Z

app/templates/_package.json

+ "build:deploy": "npm run clean && webpack --progress --colors --mode=dev --target=web",
+ "watch": "npm run clean && webpack --progress --colors --watch --mode=dev --target=web",
+ "test": "echo \"Error: no test specified\" && exit 1",
+ "zip": "bestzip dist/<%= appId %>.zip dist/*"


There was an issue with putting zip in dist. Basically bestzip also tried to zip the zip. For that reason I put zip outside of dist. See rkorytkowski/openmrs-owa-conceptdictionary@3b2de69

Thanks for the heads up!

pgutkowski · 2016-05-05T06:12:56Z

Hey @psbrandt, is there anything else You want me to amend?

pgutkowski · 2016-05-07T15:12:54Z

In further testing I have found out that keeping 2 independent entry chunks (for css and js) in one bundle file is not the best idea, scripts didn't work. Creating separate bundle for css files fixed the problem.

psbrandt · 2016-05-09T06:42:57Z

@PawelGutkowski I'll take another look at this today.

psbrandt · 2016-05-09T13:03:34Z

app/templates/_package.json

- "test": "echo \"Error: no test specified\" && exit 1"
+ "clean": "rimraf dist && rimraf coverage*",
+ "build": "webpack --progress --colors --mode=production --target=web",
+ "build:prod": "npm run clean && npm run test && npm run build && npm run zip",


Can you add a build:dev script so that it's possible to build a non-minified version for debugging?

@psbrandt Done.

psbrandt · 2016-05-09T13:41:09Z

The /img/omrs-button.png file mentioned in manifest.webapp is not included in the generated ZIP file, so when you install the app, the app icon is broken on the Manage Apps page. I guess this is because Webpack is optimising by base64 encoding and inlining the image. I think (at least for the app icon) we should keep the original PNG.

pgutkowski · 2016-05-09T15:25:34Z

I have overlooked that, fixed.

psbrandt · 2016-05-09T15:29:31Z

When I install a package, it seems like Webpack is including it in app.bundle.js, but I expect it to be in vendor.bundle.js, since it's a third party library. So, if run:

npm install --save moment

and then, if my app.js looks like:

import moment from 'moment';

(function () {
  'use strict';
  document.addEventListener("DOMContentLoaded", function(event) {
    console.log('OpenMRS Open Web App Started.');
    console.log(moment().format('YYYY-MM-DD'));
  });
}());

it works, but moment seems to be in the wrong file. Maybe my expectation is wrong?

psbrandt · 2016-05-09T15:34:05Z

I'm still trying to figure out how to install CSS-only packages (e.g. animate.css), or packages that contain both CSS and JS (e.g. Bootstrap). Have you been able to get such examples working?

pgutkowski · 2016-05-09T15:43:54Z

Do You declare moment in vendor entry point? the same as jquery/angular is?. It's necessary. About installing css, besides importing our openmrs-contrib-uicommons to openmrs-owa-conceptdictionary, I haven't try it yet. I will look into it.

psbrandt · 2016-05-09T15:53:46Z

Did You declared moment in vendor entry point?

I don't think so. All I'm doing is what I posted in the comment above. I guess you are referring to this section of webpack.config.js:

entry: {
  app : `${__dirname}/app/js/owa.js`,
  css: `${__dirname}/app/css/owa.css`,
  vendor : [
    'angular'
  ]
},

Can you update the README to explain this requirement?

pgutkowski · 2016-05-09T16:06:43Z

Yeah, that's the part I meant. I have updated README.

pgutkowski · 2016-05-09T16:35:02Z

I've got animate.css working by importing it at the end of our css file @import '../../node_modules/animate.css/animate.min.css'. There's no js file in this package which would export stylesheets, so pretty importing like import 'animate.css' is not avalaible. I will try to install Bootstrap too.

pgutkowski · 2016-05-10T07:04:05Z

Installing bootstrap by webpack is painful, but there's bootstrap-webpack, which makes it quite easy. From my little experience I have to say that unfortunately, a lots of libraries are not adjusted to be used with webpack, and need some tricks/wrap packages to be installed.

psbrandt · 2016-05-10T11:27:44Z

With the latest change, the app icon is working if I do npm run watch or npm run build:deploy, but not if I actually do a production build and upload the app via the Manage Apps page. It seems that the ZIP file is generated as follows:

openmrs-owa-test
├── app.bundle.min.js
├── css.bundle.min.js
├── dist
│   └── img
│       └── omrs-button.png
├── index.html
├── manifest.webapp
└── vendor.bundle.js

Note the extra dist directory. Can you confirm this is also happening for you?

What is the reason that you are now including openmrs-owa- in the appId when generating the ZIP file?

pgutkowski · 2016-05-10T14:06:00Z

Yes, I have the same zip structure. Thanks for noticing, we overlooked that issue in Concept Dictionary, too. I will fix it later.
What's wrong with naming ZIP file with appId?

psbrandt · 2016-05-10T14:20:30Z

What's wrong with naming ZIP file with appId?

There is nothing wrong with using the appId, but you've changed the behaviour of the build to also include "openmrs-owa-", so I'm just wondering what the reason is. Also, OpenMRS modules are named like htmlformentry-2.6.omod, not openmrs-module-htmlformentry.omod, so I think we should stick to that convention. How about we remove the openmrs-owa- from the ZIP filename and include the version from package.json just like you guys have done with the Concept Dictionary OWA?

I think after the ZIP file is corrected we can land this PR. Thanks for all the hard work! We've made it a bit more complicated to add and use different packages (because Webpack makes it more complicated), so I think having good documentation and examples would be a good thing. I will try to find the time to work on that, but PRs will always be welcome :).

pgutkowski · 2016-05-11T07:57:51Z

but you've changed the behaviour of the build to also include "openmrs-owa-", so I'm just wondering what the reason is

On master branch appId in package.json already was including 'openmrs-owa', I didn't change it. Now I see that gulpfile.js had appId without this prefix, so that's where the difference come from.

I couldn't change bestzip behaviour, so now zipping is managed from webpack.config using archiver. I am sticking to naming convention, and include version in zip name. There's one catch - when You upload zip, the adress to app contains version, eg. localhost:8080/openmrs/owa/stuffitystuff-v0.1.0/index.html.

I'm happy to help. About documentation, we are planning to create video tutorial for development using this generator and openmrs-contrib-uicommons. If You got any certain ideas to improve documentation, ping me, I will certainly help!

psbrandt · 2016-05-11T08:32:26Z

@rkorytkowski, @sunbiz, how do you feel about including the version in the URL? Doing it this way would allow us to have multiple versions of the same app installed.

rkorytkowski · 2016-05-17T12:28:02Z

@psbrandt, personally I wouldn't include the version in the URL. It will be troublesome to handle when adding links to an app from other apps unless we can provide a default version with URL consistent between versions. I think the benefits do not justify the complexity behind supporting multiple versions of an app.

psbrandt · 2016-05-17T12:49:37Z

Okay @rkorytkowski, that reasoning makes sense to me. When I download the Concept Dictionary OWA, the filename has a version in it (as required by Bintray), so I suspect that if I were to install it into the OWA Module, it will have that version in the URL.

How do you think we should handle this? Whatever strategy we choose, we should definitely bake it into the generator (if applicable) sooner rather than later.

I guess a few options are:

Ask people to rename the ZIP files themselves
Add functionality to the OWA App Store to exclude the version number from the filename when downloading
Add functionality to the OWA Module to exclude the version number either when serving or when extracting the ZIP

I think my preference is probably number 3 (when extracting).

rkorytkowski · 2016-05-17T12:53:34Z

I think the filename should not matter at all to the OWA module. It should read app id from manifest.webapp and put it under correct url.

psbrandt · 2016-05-17T12:55:29Z

Agreed, so in that case we don't need to change anything here, and I think we merge this if you agree.

rkorytkowski · 2016-05-17T13:24:15Z

I agree with the merge.

psbrandt reviewed Apr 28, 2016
View reviewed changes

rkorytkowski reviewed Apr 29, 2016
View reviewed changes

psbrandt reviewed May 9, 2016
View reviewed changes

RA-1121 migrate to webpack

91610ca

RA-1121 use archiver to create distributable zip

eb6d583

psbrandt merged commit e19c74e into psbrandt:master May 17, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RA-1121/OMRSJS-5 migrate to webpack #15

RA-1121/OMRSJS-5 migrate to webpack #15

pgutkowski commented Apr 28, 2016

pgutkowski commented Apr 28, 2016 •

edited

Loading

pgutkowski commented Apr 28, 2016

psbrandt Apr 28, 2016

psbrandt commented Apr 28, 2016

psbrandt Apr 28, 2016

pgutkowski commented Apr 29, 2016 •

edited

Loading

rkorytkowski Apr 29, 2016

pgutkowski May 1, 2016 •

edited

Loading

pgutkowski commented May 5, 2016

pgutkowski commented May 7, 2016

psbrandt commented May 9, 2016

psbrandt May 9, 2016

pgutkowski May 9, 2016

psbrandt commented May 9, 2016

pgutkowski commented May 9, 2016

psbrandt commented May 9, 2016

psbrandt commented May 9, 2016

pgutkowski commented May 9, 2016 •

edited

Loading

psbrandt commented May 9, 2016

pgutkowski commented May 9, 2016

pgutkowski commented May 9, 2016 •

edited

Loading

pgutkowski commented May 10, 2016

psbrandt commented May 10, 2016

pgutkowski commented May 10, 2016

psbrandt commented May 10, 2016

pgutkowski commented May 11, 2016

psbrandt commented May 11, 2016

rkorytkowski commented May 17, 2016

psbrandt commented May 17, 2016

rkorytkowski commented May 17, 2016

psbrandt commented May 17, 2016

rkorytkowski commented May 17, 2016

RA-1121/OMRSJS-5 migrate to webpack #15

RA-1121/OMRSJS-5 migrate to webpack #15

Conversation

pgutkowski commented Apr 28, 2016

pgutkowski commented Apr 28, 2016 • edited Loading

pgutkowski commented Apr 28, 2016

psbrandt Apr 28, 2016

Choose a reason for hiding this comment

psbrandt commented Apr 28, 2016

psbrandt Apr 28, 2016

Choose a reason for hiding this comment

pgutkowski commented Apr 29, 2016 • edited Loading

rkorytkowski Apr 29, 2016

Choose a reason for hiding this comment

pgutkowski May 1, 2016 • edited Loading

Choose a reason for hiding this comment

pgutkowski commented May 5, 2016

pgutkowski commented May 7, 2016

psbrandt commented May 9, 2016

psbrandt May 9, 2016

Choose a reason for hiding this comment

pgutkowski May 9, 2016

Choose a reason for hiding this comment

psbrandt commented May 9, 2016

pgutkowski commented May 9, 2016

psbrandt commented May 9, 2016

psbrandt commented May 9, 2016

pgutkowski commented May 9, 2016 • edited Loading

psbrandt commented May 9, 2016

pgutkowski commented May 9, 2016

pgutkowski commented May 9, 2016 • edited Loading

pgutkowski commented May 10, 2016

psbrandt commented May 10, 2016

pgutkowski commented May 10, 2016

psbrandt commented May 10, 2016

pgutkowski commented May 11, 2016

psbrandt commented May 11, 2016

rkorytkowski commented May 17, 2016

psbrandt commented May 17, 2016

rkorytkowski commented May 17, 2016

psbrandt commented May 17, 2016

rkorytkowski commented May 17, 2016

pgutkowski commented Apr 28, 2016 •

edited

Loading

pgutkowski commented Apr 29, 2016 •

edited

Loading

pgutkowski May 1, 2016 •

edited

Loading

pgutkowski commented May 9, 2016 •

edited

Loading

pgutkowski commented May 9, 2016 •

edited

Loading