malaporte / nashorn-commonjs-modules

CommonJS modules support for Nashorn
MIT License
108 stars 31 forks source link

Allow properties to be set on exports object #16

Closed kipz closed 7 years ago

kipz commented 7 years ago

The following code:

Object.defineProperty(exports, "__esModule", { value: true });

does not work in Nashorn, leading to an Exception:

Caused by: .atomist/editors/SimpleEditor.js:10 TypeError: [object global] is not an Object
    at jdk.nashorn.internal.runtime.ECMAErrors.error(ECMAErrors.java:57)
    at jdk.nashorn.internal.runtime.ECMAErrors.typeError(ECMAErrors.java:213)
    at jdk.nashorn.internal.runtime.ECMAErrors.typeError(ECMAErrors.java:185)
    at jdk.nashorn.internal.runtime.ECMAErrors.typeError(ECMAErrors.java:172)
    at jdk.nashorn.internal.objects.Global.checkObject(Global.java:2061)
    at jdk.nashorn.internal.objects.NativeObject.defineProperty(NativeObject.java:286)
    at jdk.nashorn.internal.scripts.Script$Recompilation$9$60AAAAA$SimpleEditor.L:1(.atomist/editors/SimpleEditor.js:10)
    at jdk.nashorn.internal.runtime.ScriptFunctionData.invoke(ScriptFunctionData.java:647)
    at jdk.nashorn.internal.runtime.ScriptFunction.invoke(ScriptFunction.java:494)
    at jdk.nashorn.internal.runtime.ScriptRuntime.apply(ScriptRuntime.java:393)
    at jdk.nashorn.api.scripting.ScriptObjectMirror.call(ScriptObjectMirror.java:117)
    at com.coveo.nashorn_modules.Module.compileJavaScriptModule(Module.java:298)
    at com.coveo.nashorn_modules.Module.compileModuleAndPutInCache(Module.java:259)
    at com.coveo.nashorn_modules.Module.loadModuleAsFile(Module.java:182)
    at com.coveo.nashorn_modules.Module.attemptToLoadFromThisFolder(Module.java:157)
    at com.coveo.nashorn_modules.Module.require(Module.java:98)

This is because our exports object is a Nashorn Bindings object, and Nashorn only supports defining properties on pure JS objects (those that extend ScriptObject - an internal Nashorn class).

We are experiencing this issue due to a change in the TypeScript compiler:

https://github.com/Microsoft/TypeScript/issues/14351

but it's not uncommon for other JS processing tools to do the same:

https://duckduckgo.com/?q=Object.defineProperty(exports%2C+%22__esModule%22%2C+%7B+value%3A+true+%7D)%3B&atb=v51-6_c&ia=web

and of course, it's totally valid JavaScript

Ultimately, we may want to consider doing this for all global vars, but this seemed like a much more invasive change to the design.

kipz commented 7 years ago

Cause of https://github.com/coveo/nashorn-commonjs-modules/issues/15

malaporte commented 7 years ago

Thanks for the tip about ScriptObject - that's the thing I was missing!! I took the liberty of making a broader fix in this PR: https://github.com/coveo/nashorn-commonjs-modules/pull/17

I copied the test you included here and it's now passing.

malaporte commented 7 years ago

(btw I created a new PR because I know of no way to "extend" on yours, sorry if it's poor GH "etiquette").

kipz commented 7 years ago

Fixed by #17