fengari-lua / fengari

🌙 φεγγάρι - The Lua VM written in JS ES6 for Node and the browser
MIT License
1.8k stars 64 forks source link

luaconf.js: ReferenceError: process is not defined #129

Open silvasur opened 6 years ago

silvasur commented 6 years ago

When trying to use src/luaconf.js unmodified in a browser environment, you will get an error like "ReferenceError: process is not defined", since the global process is not defined in a browser environment.

This can easily be fixed by checking typeof process !== "undefined" (as it is already done in other places of fengari):

diff --git a/src/luaconf.js b/src/luaconf.js
index 387e813..422bbfd 100644
--- a/src/luaconf.js
+++ b/src/luaconf.js
@@ -1,6 +1,6 @@
 "use strict";

-const conf = (process.env.FENGARICONF ? JSON.parse(process.env.FENGARICONF) : {});
+const conf = (typeof process !== "undefined" && process.env.FENGARICONF ? JSON.parse(process.env.FENGARICONF) : {});                                                                    

 const {
     LUA_VERSION_MAJOR,
daurnimator commented 6 years ago

So this is a bit tricky, we need to be able to pass FENGARICONF at webpack build time. See https://github.com/fengari-lua/fengari-web/blob/815fdcf8d3b03f13f70033f312ea523aae57cda1/webpack.config.js#L37 Adding typeof process !== "undefined" would prevent it from working.

silvasur commented 6 years ago

Ah okay, I see. Never mind then.

daurnimator commented 6 years ago

Wait no.... I just realised that in webpack we set it to undefined anyway. So we can add it after all.

However that's just for fengari-web's use case. For anyone actually using FENGARICONF for a custom build it would break :(

silvasur commented 6 years ago

I'm pretty new to webpack, so this might be a silly idea, but the webpack config file is just JS code, right? That means we could read the FENGARICONF in the config file and build appropriate definitions for the DefinePlugin.

With these patches to fengari and fengari-web, this works just fine:

diff --git a/webpack.config.js b/webpack.config.js
index babb5bc..4d1efd5 100644
--- a/webpack.config.js
+++ b/webpack.config.js
@@ -2,6 +2,20 @@

 const webpack = require('webpack');

+function fengariconfDefine() {
+   let define = {
+       "process.env.FENGARICONF": "void 0",
+       "typeof process": JSON.stringify("undefined")
+   };
+
+   if (process.env.FENGARICONF) {
+       let fengariconf = JSON.parse(process.env.FENGARICONF);
+       define["__FENGARICONF__"] = JSON.stringify(fengariconf);
+   }
+
+   return new webpack.DefinePlugin(define);
+}
+
 module.exports = [
    {
        /*
@@ -33,10 +47,7 @@ module.exports = [
            ]
        },
        plugins: [
-           new webpack.DefinePlugin({
-               "process.env.FENGARICONF": "void 0",
-               "typeof process": JSON.stringify("undefined")
-           })
+           fengariconfDefine()
        ]
    },
    {
@@ -54,10 +65,7 @@ module.exports = [
        devtool: 'hidden-source-map',
        node: false,
        plugins: [
-           new webpack.DefinePlugin({
-               "process.env.FENGARICONF": "void 0",
-               "typeof process": JSON.stringify("undefined")
-           })
+           fengariconfDefine()
        ]
    }
 ];

diff --git a/src/luaconf.js b/src/luaconf.js
index 387e813..b1ced5c 100644
--- a/src/luaconf.js
+++ b/src/luaconf.js
@@ -1,6 +1,12 @@
 "use strict";

-const conf = (process.env.FENGARICONF ? JSON.parse(process.env.FENGARICONF) : {});
+const conf = (typeof __FENGARICONF__ !== "undefined"
+       ? __FENGARICONF__
+       : (typeof process !== "undefined" && process.env.FENGARICONF
+               ? JSON.parse(process.env.FENGARICONF)
+               : {}
+       )
+);

 const {
     LUA_VERSION_MAJOR,

(Admittedly, it's a bit ugly that this basically defines a new global variable __FENGARICONF__, but I think it's okay; a naming conflict with that name seems very unlikely.)

Now you can run FENGARICONF='{"LUA_COMPAT_FLOATSTRING": true}' npm run build and get a fengari-web.js, that prints 0 when running print(0.0). Also, the FENGARICONF environment still works when using fengari-node-cli and you don't get the ReferenceError: process is not defined error any more :)

daurnimator commented 6 years ago

(Admittedly, it's a bit ugly that this basically defines a new global variable FENGARICONF, but I think it's okay; a naming conflict with that name seems very unlikely.)

Doesn't that new global result in failure in strict mode? + warnings from linters?

silvasur commented 6 years ago

Doesn't that new global result in failure in strict mode? + warnings from linters?

Linters, yes. eslint gives me an no-undef - '__FENGARICONF__' is not defined warning (you could add an /* global __FENGARICONF__ */ at the top of the file to suppress this error).

But since the read access is guarded by typeof __FENGARICONF__ !== "undefined" this will not result in an error.

daurnimator commented 5 years ago

But since the read access is guarded by typeof __FENGARICONF__ !== "undefined" this will not result in an error.

When webpack does it's thing, I think it only replaces the uses, not the typeof checks (unless you provide that replacement as well); which means we sort of don't want that check. This is the same issue I pointed at in my first comment.