LiquidPlayer / liquidcore-cli

Command line interface for LiquidCore
MIT License
3 stars 0 forks source link

Is there a reason metro was used over webpack/browserify? #1

Open j0j00 opened 5 years ago

j0j00 commented 5 years ago

Hi @ericwlange,

I'm currently trying to bundle my javascript using liquidcore-cli's bundle command, but it doesn't currently work because metro tries to resolve built-in libs like vm, os to actual node_modules.

The error message it gives was:

UnhandledPromiseRejectionWarning: Error: Unable to resolve module `vm` from `/path/to/script.js`: Module `vm` does not exist in the Haste module map

And from looking at the metro documentation, there doesn't seem to be an easy way to stop it from resolving those modules.

Webpack should handle this out of the box, so that's what I'm planning to use. I just wanted to know if there was a specific reason you deprecated the liquidserver, in favour of metro even though it seems like it already addressed this specific issue?

ericwlange commented 5 years ago

I have come to seriously regret my decision.

I ran into all the same issues as you. I have nearly bent Metro to my will to resolve all of these issues and will check in my changes in the next few days. Try it out if you have time.

My reasoning for using Metro was simple: React Native uses it and I want LiquidCore to be compatible with RN. Unfortunately, RN uses (on Android anyway) an ancient version of JavaScriptCore which requires heavy transpilation. LiquidCore uses Node 8.9, so it is pretty modern and doesn't need all the ES6 -> ES5 stuff, plus it allows RN to run natively on 64-bit processors. Metro is just over-optimized and over-engineered for RN's faults.

I deprecated liquidserver before making sure that the node stuff didn't break. The next version of liquidcore-cli will fix that oversight.

Thanks for noting this!

j0j00 commented 5 years ago

I'd be more than happy to test :)

ericwlange commented 5 years ago

Hi @jojo-apollo, give it a try now and see if it resolves your issues. I have published it as version 0.2.0.

j0j00 commented 5 years ago

I've given it a try and it seems to be resolving the internal node libs just fine, unfortunately for my specific use-case/project setup, metro doesn't seem like it'll play fair, so I'll be using webpack instead as I don't want to keep trying to hack metro/react-native.

My project attempts to import scripts from a directory outside the project root (I know it's not best practice, but that's how it's set up), however this doesn't seem to be easily supported by Metro, so I tried using npm link instead, but that's also not supported.

From messing around trying to get the import to work, I found a small bug with the argument parser not passing in the arguments in the format that react native is expecting them, e.g. --projectRoots expects an array rather than a string. Relevant files server.js and cliEntry.js.

I wrote a crude workaround, but the best solution would be to get local-cli/server/server.js to process the arguments correctly.

Index: node_modules/liquidcore-cli/server.js
<+>UTF-8
===================================================================
--- node_modules/liquidcore-cli/server.js   (date 1545426269350)
+++ node_modules/liquidcore-cli/server.js   (date 1545426269350)
@@ -28,6 +28,22 @@

     Object.assign(args, override);

+    const specialArgTypes = [
+      {
+        'type': 'list',
+        'args' : ['assetExts', 'platforms', 'projectRoots', 'sourceExts'],
+      }
+    ];
+    specialArgTypes.forEach((obj) => {
+      obj['args'].forEach((arg) => {
+        if (obj['type'] == 'list') {
+          if (typeof args[arg] === 'string') {
+            args[arg] = args[arg].split(',');
+          }
+        }
+      });
+    });
+
     const space = (text, spaces) => {
       spaces = Math.max(0, spaces - text.length);
       for (var i=0; i<spaces; i++) text += ' ';

It also doesn't seem to read from the metro.config.js file, tbh these aren't a priority as I doubt most users will ever need to use them (I certainly don't).

ericwlange commented 5 years ago

Thanks for the detailed feedback. I haven't really been using command line arguments outside of simple things like --reset-cache, --platform=, --dev= and --minify. I should probably have tested to make sure I didn't break the arguments. I'll do that.

The only (and I do mean only -- I tried everything) way I could find to include modules outside node_modules is through the config.extraNodeModules parameter which performs a replacement. This is how I handle the native node includes. If you look in Config.js, I have put all of my ugly hacks there including this one.

Good point on ignoring metro.config.js. This is easily fixable (and an easier place to handle extraNodeModules). I will support that too.

Thanks again for giving it a try!