NeilFraser / JS-Interpreter

A sandboxed JavaScript interpreter in JavaScript.
Apache License 2.0
1.96k stars 352 forks source link

Make js-interpreter ready for node and NPM #267

Open mercmobily opened 4 months ago

mercmobily commented 4 months ago

I am currently maintaining https://github.com/mobily-enterprises/js-interpreter. It's available on npm as js-interpreter-npm. All it does, it repackage acorn.js and js-interpreter.js, but adding this at the BEGINNING of js-interpreter.js:

var globalThis
if (typeof exports === 'undefined') {
  globalThis = window
} else {
  globalThis = exports
  globalThis.acorn = require('./acorn.js')

  // This is important as JS-Interpreter assumes that the "global" scope
  // includes primitive variable types
  globalThis.String = String
  globalThis.Date = Date
  globalThis.Boolean = Boolean
  globalThis.Number = Number
  globalThis.RegExp = RegExp
}

This essentially makes sure that the module works 100% both when loaded in the browser, and when it's "required" by node. Please note that I had to add those primitive data types to the "global scope" for node.

It is a bit of a hack, but it works. Please note that I carefully avoided complex "building" steps with webpack, etc.

I would love some input on this. Really, I just have a question:

1) are you guys willing to include some changes so that js-interpreter is ready for node, rather than people using my silly wrapper that becomes outdated as you go further with development?

2) are you guys willing and happy to publish to NPM, if you do go with (1)?

cpcallen commented 4 months ago

I can't speak for Niel, who has ultimate discretion over this repo, but he has been known to listen to my advice from time to time. Here is what I would advise:

Then rebuilding and publishing would consist of ./compile.sh (possibly via npm run build, if we want to push the boat out) followed updating the version number in package.json then by npm publish and finally git commit.

I'd be happy to assist with reviewing any of this if @NeilFraser would like.

mercmobily commented 4 months ago

Jesus... you guys are not going to believe this. I have done precisely what you wrote above. There is no need to assist -- I've already done exactly what you wrote above. In fact, I did it last night, about 10 hours ago. Have a look at this repo if you don't believe me! https://github.com/mobily-enterprises/js-interpreter-esm This was done BEFORE I read your message. I find it a little astonishing...

Specificaly:

JS Interpreter is so widely used that it is bad that there is no official NPM package. We know there are a number of unofficial packages and these do not necessarily get updated promptly when issues are fixed. There should be an official js-interpreter package published from this repository and updated promptly when changes are made here.

Precisely what I was going to write. Most people today will install the NPM module js-interpreter. If this package starts distributing an ESM version, as well as CommonJS, then I think it would be fair to ask the owner of the npm module js-interpreter to please hand it over so that js-interpreter can use that namespace (which would be ideal). I am not sure how the owner feels about it...

I believe the owner of the existing js-interpreter package has offered it to Neil for official use, but if not another name could be chosen.

Ah, THAT'S how he feels about it :-D I would ask him to rename his module js-interpreter-cli, since once we do this, the only difference between his and the official one is the presence of the CLI. We might link to his module too.

To facilitate packaging JS Interpreter as an NPM package, compile.sh should be modified to produce something that can be ingested as a module. Probably the most conservative option would be to use Closure Compiler's --output_wrapper switch to add a very minimal UMD-style wrapper along the lines of what @mercmobily proposes above, but preferably with the compiled code actually wrapped in an IIFE as then --assume-function-wrapper will allows additional optimisation without contaminating the global namespace.

I am for simple solutions. I don't think we need to go as far as using the Closure Compiler, frankly. What we need is so simple... really. If you look at the repo (which is a TOTALLY UNTESTED sketch, I put it together last night before going to sleep), you will see that all we really need is two lines of code to sandwitch the module with. I am all for simple and elegant solutions. Please give me a day or so to make sure what I did last night actually works. If it does, it really does everything we need.

I also wrote "please let's drop AMD support,

I don't think there is any reason to support AMD modules any more.

Ah that was another point I was going to make. Fantastic.

If we want to be a little more futurepresent-looking, it could instead emit an ESM, but then it the package would need separate ESM and script entrypoints.

It will need to. My sketched package.json file has already this:

  "main": "JS-Interpreter.js",
  "module": "JS-Interpreter-esm.js",

A package.json should be added specifying that the package content is more or less just acorn_interpreter.js, with an appropriate exports section.

Yep.

Then rebuilding and publishing would consist of ./compile.sh (possibly via npm run build, if we want to push the boat out) followed updating the version number in package.json then by npm publish and finally git commit.

Precisely.

I'd be happy to assist with reviewing any of this if @NeilFraser would like.

Let me check whether my sketch works. I mainly pushed it as is just to show how aligned our ideas are. The main difference is in the way we compile to ESM -- I think it's very trivial to do so without a complex, full-on building stage. But, I need to check that too.

Once what I did works, I will send a PR over if that's OK.

Thank you!

mercmobily commented 4 months ago

https://github.com/NeilFraser/JS-Interpreter/pull/268 does this. Maye this issue should be closed?