Skip to content

Added initial CommonJS support #270

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 1 commit into from
Closed

Conversation

jpg0
Copy link

@jpg0 jpg0 commented Oct 21, 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 provide isolation from libraries' adding to the global namespace, by loading each library into a script engine with it's own context, then referencing the exports inside it
  • 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

I do believe that pulling the loading engine up into OH code would be better, but agree that we should wait for JDK9/ES6 before we do this (although if it really drags on, I may do it). Either way, this PR means that all library code written now would be compatible with that future.

@jpg0
Copy link
Author

jpg0 commented Oct 21, 2019

Oh, and one other thing as mentioned elsewhere: this really shows that utils.js should be broken up into logical parts. It seems to cover a little bit of everything. For example one of the first things that I'd do is move getItem, sendCommand, postUpdate to items.js (#266)

@jpg0
Copy link
Author

jpg0 commented Oct 22, 2019

After looking at this, I have discovered that there is a problem:

  • Isolation happens by loading scripts into a new context, in a new engine
  • This generally works fine
  • However scriptExtension.importPreset isn't present in these contexts
  • I can however pre-load all the properties from these and supply them into sub-contexts via synthetic modules (then they use via require('RuleSupport') for example).
  • This appears to not work well however, causing issues calling functions back in the parent context and marshalling arguments through
  • Ultimately, I think providing isolation this way is too complex and needs to be done in the above OH layer.
    I'll close this PR and open a new one without isolation.

@jpg0 jpg0 closed this Oct 22, 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.

1 participant