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

[Feature Request] Update Javscript code to ES6 and function syntax, add ESLint/JSLint code enforcment to the project #195

Open
TareqK opened this issue Jul 27, 2019 · 11 comments

Comments

@TareqK
Copy link

TareqK commented Jul 27, 2019

Pretty self explanatory, It would be nice to re-write the JavaScript extensions in es6 syntax, because the class-based syntax there is cleaner to work with. Adding ES/JSLint is also a pretty good idea, to enforce method naming, documentation, and class conventions.

I can see if i can try to do some of the es6 work, but i dont want to start working on it if the community isnt on board with it.

@TareqK TareqK changed the title Updates Javscript code to ES6 class and function syntax, add ESLint/JSLint code enforcment to the prject Update Javscript code to ES6 and function syntax, add ESLint/JSLint code enforcment to the prject Jul 27, 2019
@TareqK TareqK changed the title Update Javscript code to ES6 and function syntax, add ESLint/JSLint code enforcment to the prject Update Javscript code to ES6 and function syntax, add ESLint/JSLint code enforcment to the project Jul 27, 2019
@TareqK TareqK changed the title Update Javscript code to ES6 and function syntax, add ESLint/JSLint code enforcment to the project [Feature Request] Update Javscript code to ES6 and function syntax, add ESLint/JSLint code enforcment to the project Jul 27, 2019
@5iver
Copy link
Member

5iver commented Jul 27, 2019

ES6 won't be available until OH is fully compatible with at least jdk9, which is close. After that, the functionality in the Jython libraries will be migrated to JS.

This has bugged me and several times I've wanted to start in and just do it all with functions. But I can never get past the fact that there wouldn't be any decorators.

@TareqK
Copy link
Author

TareqK commented Jul 27, 2019

Can we at least add JSLint and do some documentation and cleanup the files? It would make the transition easier

@5iver
Copy link
Member

5iver commented Jul 27, 2019

Could you provide an example of what you're thinking of? We'll want @lewie 's feedback too. Any help with the JS libraries would be greatly appreciated. The goal is to have equal functionality with the JythonHLs.

@TareqK
Copy link
Author

TareqK commented Jul 28, 2019

General Stuff related to formatting, for example this block

     var mainPath 				= automationPath + 'lib/javascript/core/';
	//https://wiki.shibboleth.net/confluence/display/IDP30/ScriptedAttributeDefinition
	var logger 					=      Java.type("org.slf4j.LoggerFactory").getLogger("jsr223.javascript");
	try {
		var RuleBuilder = Java.type("org.openhab.core.automation.util.RuleBuilder");
	} catch(e) {
		var RuleBuilder = Java.type("org.eclipse.smarthome.automation.core.util.RuleBuilder");
	}

in the utils.js file should be something more like

  var mainPath = automationPath + 'lib/javascript/core/';
  var logger = Java.type("org.slf4j.LoggerFactory").getLogger("jsr223.javascript");
  try {
    var RuleBuilder = Java.type("org.openhab.core.automation.util.RuleBuilder");
  } catch(e) {
    var RuleBuilder = Java.type("org.eclipse.smarthome.automation.core.util.RuleBuilder");
   }

With cleaner indentation and less spacing. Additionally, Documentation should be re-written in the standard JSDoc Format, so something like this

// ### UpdatedEventTrigger ###
var ItemStateUpdateTrigger = function(itemName, state, triggerName){
    return ModuleBuilder.createTrigger().withId(getTrName(triggerName)).withTypeUID("core.ItemStateUpdateTrigger").withConfiguration( new Configuration({
        "itemName": itemName,
        "state": state
    })).build();
}

Would be re-written as

/**
*  Rule trigger for item state updated
* @param {string} itemName - The name of the item to listen to updates on
* @param {string} state - The state to listen for 
* @param {string} triggerName - What the trigger is called
*  @returns {Trigger} A Trigger for an event
*/
var ItemStateUpdateTrigger = function(itemName, state, triggerName){
    return ModuleBuilder.createTrigger().withId(getTrName(triggerName)).withTypeUID("core.ItemStateUpdateTrigger").withConfiguration( new Configuration({
        "itemName": itemName,
        "state": state
    })).build();
}

Which, with the JSDoc command line utility, will produce an HTML Page documenting these methods and classes and their relationships. This also helps with IDE Support, and generally helps fill out the wiki.

Resrouces :

The main benefit of doing all this would be to not mess up too too much when we move to es6, since we know beforehand what everything is supposed to do, thus avoiding the mess of porting by copy-paste instead of... ya know, using the benefits and syntatic sugars of es6+ , and of course, a living-document of documentation on the project

@5iver
Copy link
Member

5iver commented Jul 28, 2019

For formatting, cleanup and documentation... go for it! I'd prefer to use spaces rather than tabs and indent with 4 spaces.

The documentation is a little trickier though. The pages are being generated using Sphinx, but it hasn't been setup to autogenerate the JS docs yet. We probably want to use something like https://pypi.org/project/sphinx-js/ (more info here... https://hacks.mozilla.org/2017/07/introducing-sphinx-js-a-better-way-to-document-large-javascript-projects/) to pull them into the documentation pages. But a good first step would be to get the documentation into the files!

I've been hesitant on getting the JS docs setup until the JS libraries are at least split out like in the JythonHLs, since so much would change. I've got a repo where I'd started in on this, but it's such a big change that I was thinking it might be better to do along with the es6 changes and shelved it. There's also so much other stuff that I'd like to do for automation, but if you are motivated, please contribute! I still have the repo marked as Under Construction, so big changes should hopefully be expected and will not be too disruptive for people.

I would be very happy to see someone driving the JS libraries forward!

@TareqK
Copy link
Author

TareqK commented Jul 28, 2019

Reading into the sphynx-js Link you put up, it seems that it takes JSDoc anyhow, so i will be going ahead with the cleanups/docs, But expect a lot of asking questions becuase im new to JS/Java Polygot stuff

@5iver
Copy link
Member

5iver commented Jul 28, 2019

so i will be going ahead with the cleanups/docs

Fantastic! If you use the JSDoc format, it looks like we can pull them into the documentation pages. If you want to get into writing examples and things, then it should be done using reST. You can find some information for that here... https://openhab-scripters.github.io/openhab-helper-libraries/Contributing/Writing%20Docs.html, but you can also pick up a lot looking at the existing documents under the Sphinx directory in the repo.

But expect a lot of asking questions becuase im new to JS/Java Polygot stuff

I'd be worried if you didn't ask questions! The only stupid questions are the ones that aren't asked!

@TareqK
Copy link
Author

TareqK commented Jul 29, 2019

Well Ive looked at the code and did some cleanups to the spacing and whatnot, but before i start documenting, i need a bit more info about a couple of things

  1. Where in the openhab java code are scripts called, and variables bound? I know that there needs to be a place where java variables that are called as globals in scripts need to be bound, cant seem to find where tho

  2. function((context){}(this)) i assume also has something to do with binding, where is the context bound as well?

  3. can in change the names of stuff to be more language-appropriate?

@5iver
Copy link
Member

5iver commented Jul 29, 2019

Where in the openhab java code are scripts called, and variables bound?

This shouldn't be pertinent to the helper library documentation, but...

loading...

https://github.com/openhab/openhab-core/blob/master/bundles/org.openhab.core.automation.module.script/src/main/java/org/openhab/core/automation/module/script/internal/ScriptEngineManagerImpl.java#L147

scope values depend on the ScriptEngineFactory used, but here is Nashorn...

https://github.com/openhab/openhab-core/blob/master/bundles/org.openhab.core.automation.module.script/src/main/java/org/openhab/core/automation/module/script/internal/NashornScriptEngineFactory.java#L58

function((context){}(this)) i assume also has something to do with binding, where is the context bound as well?

I'm not sure what you're asking, but it sounds like a qestion about self executing anonymous functions. There are some helpful links in #171 and lots on the internet.

can in change the names of stuff to be more language-appropriate?

All variables and functions will be migrated to snake_case to avoid conflicts with OH, Java, and the future Scripting API. I wouldn't change code until after the JS libraries are reorganized to match the Jython libraries. That's what I was trying to communicate earlier. It is going to be difficult to document something that is all going to be changed. Two easy places to start would be metadata.js and osgi.js, which were recently added.

Another thought... please submit one PR per file so that there aren't a mass of changes going into one PR.

@lewie
Copy link

lewie commented Jul 31, 2019

I'm a friend of tabs, but this is an idle discussion. I don't care about the formatting, as long as you use empty or tabs uniformly (best with auto-format in VS Code).

@TareqK, I'm happy about everyone who is involved in the JS area, I can't get involved at the moment.
If it works with JSLint and you take over the implementation in ES6. Why not, then I like it very much!

@jpg0
Copy link

jpg0 commented Nov 27, 2019

@TareqK if you're still interested in this:

Happy for any contribution; I am hoping that this will make it back to this this project once it can be accepted!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants