openhab-scripters / openhab-helper-libraries

Scripts and modules for use with openHAB
Eclipse Public License 1.0
88 stars 69 forks source link

Added initial (unisolated) CommonJS support #271

Closed jpg0 closed 4 years ago

jpg0 commented 4 years ago

PR as discussed in #267

This implements a CommonJS module system for loading libraries.

To provide isolation, we should implement module loading in the OH java layer. Either way, this PR means that all library code written now would be compatible with that future.

Martin-Stangl commented 4 years ago

One thing utils.js does right now is to override console.log and the other console. logging functions. With the unisolated approach, log.js probably still could do that easily. But would it still be possible with an isolation? But anyway, I did not move it (console.) to log.js, as I am not sure it is needed.

Simply removing 'me' from the uuid is probably the proper solution anyway. I do not know why it was in there, but ideally, an ID has no other functionality beside identifying something (across of all spacetime). Everything else added usually requires additional processes at some point. (I can elaborate lengthy on that, if needed :-).) There could be a need for debugging purposes, if the debug logs do not give you anything else besides IDs, but you could probably still look up somewhere, wich trigger ist is. And on top of that, the current implementation allows for providing own IDs (parameter triggerName) anyway.

jpg0 commented 4 years ago

With isolation, it would not be possible (unless we provided an explicit 'backdoor' for this), without it would work fine (although creating side-effects as a result of a require would be bad practice).

Completely agree re: me - I suspect it was left in there to assist with debugging when building the libraries (and TBH there's a lot of things in there like this which need cleaning up).

jpg0 commented 4 years ago

I just tried adding my first true 3rd party module - cronstrue. (This is a library to convert cron expressions to human-readable form, which I use for labels as I generate items from the rules to toggle all schedules on/off and they need a good name.)

I needed to make a small change to the init.js code to support it - to ensure that module.exports is returned as well as exports, but I regard that as a bug as it should have always done so (like NodeJS). But given this, I could import (well, cut & paste, we're a long way off npm) the minified code and it all worked fine! I regard this as a huge win, especially as this is a typescript project and if you look at the distributed JS you'd have a very hard time trying to rework it to use in the existing system.

One thing to note: I included an Object.assign polyfill into init.js. I assume we don't have a policy on polyfills yet.

jpg0 commented 4 years ago

I'm going to close this PR as I've moved quite a bit further ahead: