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

Support NodeJS #46

Merged
merged 5 commits into from Oct 4, 2018

Conversation

Projects
None yet
2 participants
@Cobertos
Contributor

Cobertos commented Sep 30, 2018

This PR utilizes the support that NodeJS added in v10.5.0 for Workers to allow LoaderWorkerSupport and subsequently WWOBJLoader2 to run in a NodeJS environment.

  • Wrap all window usages in if(typeof window !== "undefined")
  • Add branch for NodeJS specific code for all Worker related code.
  • Test it out in Nodejs environment
  • Test it out in Webpack environment

Let me know if there's anything else you'd want me to add to this to fit the rest of the codebase!

@Cobertos

This comment has been minimized.

Contributor

Cobertos commented Sep 30, 2018

Tested this in NodeJS 10 and it works. I have to use parseAsync as NodeJS doesn't support XMLHttpRequest. You can polyfill XMLHttpRequest but that won't work if the files you need to load are locally on your computer (unless your polyfill supports file:// which the ones I found did not).

My original polyfill polyfills XMLHttpRequest in FileLoader.js (which this library relies on) in the default THREE.js build with the node-xmlhttprequest npm modules. I used a webpack configuration below to do this:

// Extra Webpack rules
module : {
	rules : [{
		test : /three\.module\.js$/,
		use : {
			loader: path.resolve(__dirname, 'require-loader.js'),
			options: {
				requires : {
					"XMLHttpRequest":  ["xmlhttprequest", "XMLHttpRequest"]
				}
			}
		}
	}]
}
//Custom require-loader.js to add a require(); to the top of the file
//For every key in query.require, it will assign a variable of the same name
//to require the given value. If the value was an array, the first item is
//the module to require, and the rest of the items are properties
module.exports = function(source) {
  //this.cacheable();
  var requireStr = "";
  Object.keys(this.query.requires).forEach((name)=>{
    var str = "var " + name + " = eval('require(\"";

    var requireName = this.query.requires[name];
    var requireProps = [];
    if(Array.isArray(requireName)) {
      requireProps = requireName.slice(1);
      requireName = requireName[0];
    }

    str += requireName + "\")";
    requireProps.forEach((prop)=>{
      str += "." + prop;
    });
    str += "');\n";
    requireStr += str;
  });
  return requireStr + source;
};
@kaisalmen

This comment has been minimized.

Owner

kaisalmen commented Sep 30, 2018

Looks good. I will check tomorrow and will merge if manual test are good.
Thanks in advance!

@Cobertos

This comment has been minimized.

Contributor

Cobertos commented Sep 30, 2018

I'm actually finding that this is breaking in Webpack environments when it tries to rewrite the require() statements.

I feel like it's hacky and kind of opaque, but one way is to fix this wrap the require() in eval() like, eval('require("worker_threads")');. How does this sound to you?

Otherwise, I'm going to see if more knowledgeable Webpack folks will know how to ignore specific require()s without requiring the users to use the IgnorePlugin

@kaisalmen

This comment has been minimized.

Owner

kaisalmen commented Oct 2, 2018

Doesn't it make sense to have a derived version of WorkerRunnerRefImpl just for use in nodej? If init is better encapsulated in methods then they can just be overridden.
I don't like to introduce hacks because of some bundeling technology doesn't work. This is conceptually wrong. I am not saying that I am against it, but I hope there is a better solution or lets say the default is clean, but there is a hacky extension seems better to me.
I have to admit that I have not really used webpack before, so I have no background knowledge here.

@Cobertos

This comment has been minimized.

Contributor

Cobertos commented Oct 2, 2018

@Cobertos

This comment has been minimized.

Contributor

Cobertos commented Oct 2, 2018

@Cobertos

This comment has been minimized.

Contributor

Cobertos commented Oct 4, 2018

Now there's two implementations of LoaderWorker and WorkerRunnerRefImpl in their own file just for NodeJs stuff, and those new classes inherit from their respective classes. Also, all branching statements based on environment are in WorkerSupport and there's only two, one to choose the correct LoaderWorker and one to choose the correct WorkerRunnerRefImpl.

Does this look better to you?

@kaisalmen

This comment has been minimized.

Owner

kaisalmen commented Oct 4, 2018

@Cobertos thanks for all the work. I will check tonight or on the weekend. My spare time for Open Source is currently very limited. I will provide feedback, but it may take a day or two.

@kaisalmen

This comment has been minimized.

Owner

kaisalmen commented Oct 4, 2018

I pushed an update and will merge now.
NodeLoaderWorker must be contained inside WorkerSupport otherwise LoaderWorker cannot be found. Both are in "private" scope of "WorkerSupport".
Apart from this there are only code style fixes.
Thanks!

@kaisalmen kaisalmen merged commit a0b1a27 into kaisalmen:master Oct 4, 2018

@Cobertos

This comment has been minimized.

Contributor

Cobertos commented Oct 7, 2018

I should have clarified, the code I has pushed previously was not finished/tested but more to just get your idea on if that was a better structure. I have since pushed the fixes for it to the fork this PR was made from and have tested it in Node.js and Browser

Should I make a separate PR or will reverting and updating this one suffice?

@kaisalmen

This comment has been minimized.

Owner

kaisalmen commented Oct 7, 2018

If 34e4eec and cd226fa (last two commits on your nodejs branch) are sufficient, then I can just merge them manually, you don't have to issue another PR.

Just for info: Once this is done, I will merge a branch that transforms all singletons to function/prototype definition (backport from dev branch (V3.0.0)). It puts NodeLoaderWorker and NodeWorkerRunnerRefImpl under namespace THREE.LoaderSupport.WorkerSupport and they can stay in their own file as you initially suggested. I do all this to hopefully remove a problem with broken Worker code when minification+mangling is used.

@Cobertos

This comment has been minimized.

Contributor

Cobertos commented Oct 7, 2018

@kaisalmen

This comment has been minimized.

Owner

kaisalmen commented Oct 8, 2018

Done, both merges. Please, double check on master branch nothing is broken. Thanks!

Btw, are you willing to a add a small nodejs test/example?

@Cobertos

This comment has been minimized.

Contributor

Cobertos commented Oct 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment