Don't load Node-specific code when bundled via Webpack #166
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
daurnimator
requested changes
Aug 29, 2019
| @@ -0,0 +1,7 @@ | |||
| "use strict"; | |||
|
|
|||
| const isBrowser = ((typeof process === "undefined") || process.browser); | |||
There was a problem hiding this comment.
I don't love a new source file for this. I think put it into defs.js?
|
|
||
| let lua_writestring; | ||
| let lua_writeline; | ||
| if (typeof process === "undefined") { | ||
| if (isBrowser) { |
There was a problem hiding this comment.
This check is actually checking for the process module (it uses process.stdout if available in the other branch)
| @@ -270,7 +274,7 @@ const ll_loadlib = function(L) { | |||
| }; | |||
|
|
|||
| const env = (function() { | |||
| if (typeof process !== "undefined") { | |||
| if (isNode) { | |||
There was a problem hiding this comment.
Might want to leave this one as actually checking process?
| const isBrowser = ((typeof process === "undefined") || process.browser); | ||
| const isNode = !isBrowser; | ||
|
|
||
| module.exports.isBrowser = isBrowser; |
There was a problem hiding this comment.
IIRC don't use camelCase anywhere in fengari; please use snake_case for consistency.
|
After discussing this further on IRC, I'm closing this in favor of patching |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reviewer Note:
I'm mostly just starting out on my Node/Webpack/JS/* journey. If there is a blatant misunderstanding of the inner workings of Node, Webpack, or anything related to them, please be gentle with me.
What is being fixed?
The issue is described nicely in #156. Basically, when being bundled through Webpack, Fengari would think that it's running in a Node context (because Webpack sets a global
processvariable, which Fengari checks for in a bunch of places.) This would result in Fengari trying to load Node-specific code, which, of course, fails in a Webpack bundle intended for the web.How is it fixed?
I've extracted the expression that determines whether Fengari is running in a node environment into a separate module and changed the various instances where Fengari checked for Node (or non-Node) environments to make use of this module. I've also smartened up the expression slightly.
At its core, the change is the following:
How is it tested?
I didn't find any tests for the existing behavior and didn't feel like adding a test for this change would provide much value, but I did confirm that this works in my own project.