Skip to content
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

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

Open
j0j00 opened this issue Dec 19, 2018 · 5 comments
Open

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

j0j00 opened this issue Dec 19, 2018 · 5 comments

Comments

@j0j00
Copy link

j0j00 commented Dec 19, 2018

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
Copy link
Member

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
Copy link
Author

j0j00 commented Dec 19, 2018

I'd be more than happy to test :)

@ericwlange
Copy link
Member

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
Copy link
Author

j0j00 commented Dec 21, 2018

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
Copy link
Member

ericwlange commented Dec 23, 2018

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants