Core Apps has ECMAScript 6 specific code

I see that some of the new dashboard widgets are using the “let” statement, which is only available as-of ECMAScript 6. I’m thinking we don’t want to make an ECMAScript 6-compatible browser necessary for the reference application? Or am I missing something?

https://github.com/openmrs/openmrs-module-coreapps/blob/master/omod/src/main/webapp/resources/scripts/fragments/dashboardwidgets/obsgraph/obsgraph.controller.js#L40

Thanks!

Take care, Mark

Hi @mogoodrich,

a lot of JS projects write their code in ES6 or later and use https://babeljs.io/ to transpile their JS files that uses the latest JS features into JS that can be run by older browsers. This transpiling is done in the build process (by JS tool used to build the app like npm, gulp, webpack, …). Not sure if that is done in the coreapps though. Quick search in the repo for ‘babel’ didnt show anything.

I find it hard to mix JS with Java code in one repo with only using maven as the build tool for both languages since maven is not made for JS development and thus you miss out on the wealth of JS tools. I have tried https://github.com/eirslett/frontend-maven-plugin which can help bridge this gap and talk to the JS build tools so one can transpile and also minify the JS code. Something like that might be needed at least for transpiling. I quick search in the repo also shows no tests which might stem from the setup without JS build tools. Maybe if extracting the JS code into a separate repo with only JS code and build tools is too much hassle at least using this maven plugin for the transpiling can help.

This reminds me of https://hackernoon.com/how-it-feels-to-learn-javascript-in-2016-d3a717dd577f

As an arbitrary guideline I would say we should that we want the Reference Application to run on 3 year old browser versions.

Peeking at let - JavaScript | MDN I see that “let” is only supported in Firefox from Jan 2016. So, not 3 years old.

In general I think:

  • We should move to modern JavaScript for writing code
  • We should transpile using babel for browser compatibility

This should definitely get addressed before the RefApp release this month. (FYI @raff.)

“let” should not have sneaked through to the newly written controllers. I’ll get rid of that.