Skip to content

Added initial (unisolated) CommonJS support #271

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

Closed
wants to merge 2 commits into from

Conversation

jpg0
Copy link

@jpg0 jpg0 commented Oct 22, 2019

PR as discussed in #267

This implements a CommonJS module system for loading libraries.

  • requiring and exporting work just like you would expect
  • this implementation does NOT provide isolation from libraries' adding to the global namespace
  • I have not added a version of log.js (as in Initial contribution core/log.js #268), although this is trivial to port
  • I have removed the requirement for defining 'me' as it's incompatible here, and simply used the uuid

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
Copy link

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.

… exports via a module; added Object.assign polyfill
@jpg0
Copy link
Author

jpg0 commented Oct 23, 2019

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
Copy link
Author

jpg0 commented Oct 23, 2019

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
Copy link
Author

jpg0 commented Nov 6, 2019

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

  • I've got ES6 working via GraalJS - although not ES6 modules :(
  • I've got a fully nodeJS-compliant (isolated) CommonJS loader implementation moved up into the OH Java plugin code.
    Hopefully this will be accepted into OH and will then be available here.

@jpg0 jpg0 closed this Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants