[HOLD for payment 2021-09-17] [DEV] The check port script fails on it's own and does not check if the process running on the port is a metro bundler

[dklymenk]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

1. Script Fails on it's own

Action Performed:

1. Make sure you are not running anything on port 8081
2. Try to run npm run android

Expected Result:

The check port script passes and continues execution

Actual Result:

The script reports that port 8081 is used. Possibly because it tries to start 3 servers on the same port at the same time.

Workaround:

Run npx react-native run-android manually to avoid running the check port script

Platform:

Where is this issue occurring?

• GNU/Linux (Arch Linux)

2. Script does not check if port is used by bundler

Action Performed:

1. Start metro bundler npm start
2. npm run android

Expected Result:

The check port scripts doesn't error out if port 8081 is already used by metro bundler

Actual Result:

The script reports that port 8081 is used and prevents further execution

Workaround:

Run npx react-native run-android manually to avoid running the check port script

Platform:

Where is this issue occurring?

• GNU/Linux (Arch Linux)
• macOS Big Sur

Version Number:
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:

View all open jobs on GitHub

[MelvinBot]

Triggered auto assignment to @Christinadobrzyn (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[dklymenk]

Proposal

According to #3718, the intention behind that script is to improve the error message when attempting to run the iOS/android app alongside the desktop and web in development, but while it does that to a certain extent, it removes some of the functionality baked into any react-native project by default. To be more specific, the ability to start the bundler manually (issue 2 in the original post).

Why is it important? The npx react-native run-{ios,android} commands can't spawn the window with a sub-process in every environment. There is this issue for example facebook/react-native#28807 and while it works for me on macos in tmux, I would assume that it won't be the case with WSL. So in cases like this it is recommended to start metro bundler manually.

I have a proposal that both allows you to keep that fancy error message and brings back the default functionality.

The original check port script tries to start 3 servers on the port at the same time. I think it fails for me because it tries to start one of it's servers while another one is still occupying the port. Even if we are to fix this issue, we still need to check if the port is used by metro bundler and it's not possible with this approach.

I propose using a function isPackagerRunning from @react-native-community/cli-tools. You can find the source here: https://github.com/react-native-community/cli/blob/master/packages/tools/src/isPackagerRunning.ts

Basically it has 3 possible returns:

• running - if metro bundler is already on that port
• not_running - if nothing is running on that port
• unrecognized - if there is a server that is not metro bundler on that port

So we can replace the entire start-three-servers-at-the-same-time code with this single call. It should looks like this:

isPackagerRunning().then((result) => {
    if (result === 'unrecognized') {
        console.error(
            'The port 8081 is currently in use.',
            'You can run `lsof -i :8081` to see which program is using it.\n',
        );
        process.exit(1);
    }
});

If you still want to use it for any port you can pass a port to it:

// The code from original implementation
const inputPort = Number(process.argv.slice(2)[0]);
if (Number.isNaN(inputPort) || inputPort < 0 || inputPort > 65535) {
    console.error('This command requires an argument that must be a number [0-65535]\n');
    process.exit(1);
}

isPackagerRunning(inputPort).then((result) => {
    if (result === 'unrecognized') {
        console.error(
            `The port ${inputPort} is currently in use.`,
            `You can run \`lsof -i :${inputPort}\` to see which program is using it.\n`,
        );
        process.exit(1);
    }
});

With this implementation you still get the same error if port is used by anything that doesn't look like metro bundler and you don't have to kill everything on port 8081 before running npm run {android,ios}.

[dklymenk]

Oh, and since this is a dev-only issue that nobody has complained about since that check-port script was merged, I am willing to contribute this one without an upwork contract. It doesn't really make sense for you to pay for something that only apparently affects one person.

Thanks.

[MelvinBot]

Triggered auto assignment to @tgolen (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[tgolen]

That proposal sounds mostly good to me, and thank you for volunteering to fix it! The one question I have is how are you going to access that isPackagerRunning method. Are you going to make a copy from the source, or are you going to install the source via npm and import it?

cc @HorusGoul @thienlnam Since you wrote the script initially, it might be nice to have you weigh-in on this proposal.

[dklymenk]

The one question I have is how are you going to access that isPackagerRunning method. Are you going to make a copy from the source, or are you going to install the source via npm and import it?

I certainly forgot to add this to the code examples above, but I would require it just like anything else in node.

const {isPackagerRunning} = require('@react-native-community/cli-tools');

We don't have to install anything, we already have all the needed node modules in the project. In fact, commands npm run {android,ios} use this exact function under the hood to check whether a metro bundler should be started.

[HorusGoul]

That sounds great! I'm on board with these changes.

[dklymenk]

Ok, great.

I have a few questions:

1. Do we need to keep the port flexible? As far as I can tell, the script is not used for anything but 8081. I would rather keep the script as simple as possible.
2. Shall we change the script name? Even if we are not removing the port argument, I think the name should reflect the fact that the script is now checking if metro bundler is started or can be started. So, for example, something like checkMetroAvailability or checkMetroPort or checkMetroCompatibility sounds better to me, but they don't actually showcase the fact that it passes under 2 conditions (running or not_running), so your name suggestions are welcome. I don't have any hard opinions on this one, but I think it would be better if the name is descriptive enough, so that a contributor checking out scripts in package.json doesn't have to look at the script implementation to tell what it does.

[HorusGoul]

1. I would assume that the port it's always 8081.
2. I like the checkMetroPort one! Maybe we could replace the word metro with Bundler or Packager? I don't know.

Let's see what the others think.

[tgolen]

I agree with Horus that the port doesn't need to be flexible at this point (we can always adjust it later if we need to). I'd be OK with a name change as well, and I do not have a strong opinion about what it changes to. I think all suggestions have been fine.

cc @Christinadobrzyn I am giving the official 🟢 to hire @dklymenk for this job.

[Christinadobrzyn]

Posted in Upwork!

Internal post - https://www.upwork.com/ab/applicants/1431066549132935168/job-details
External post - https://www.upwork.com/jobs/~01a8053e60a1e89c37

Hired @dklymenk!

[roryabraham]

Hey @dklymenk, I completely agree that the check-port solution we have now is not a great developer experience. Before you submit a pull request, I wanted to propose an alternate/complementary solution that was discussed briefly in an internal issue. In particular, I wrote up the following proposal to basically take the desktop start script and make it cross-platform:

1. Create a Node.js script at the project root called start.js that looks something like this:

   #!/usr/bin/env node
   const yargs = require('yargs');
   const portfinder = require('portfinder');
   const concurrently = require('concurrently');
   
   const argv = yargs
       .options({platform: {type: 'string', demandOption: true}})
       .check((args) => {
           if (!['web', 'desktop', 'ios', 'android'].includes(args.platform.toLowerCase())) {
               throw new Error(`Error: ${args.platform} is not a supported platform.`);
           }
           return true;
       })
       .argv;
   
   const {
       platform,
       ...args
   } = argv;
   
   const basePort = (platform === 'desktop' || platform === 'web') ? 8080 : 8081;
   portfinder.getPortPromise({
       port: basePort,
   }).then((port) => {
       const platformProcesses = {
           desktop: [
               {
                   command: `webpack-dev-server --config config/webpack/webpack.dev.js --port ${port} --platform desktop ${args}`,
                   name: 'Renderer',
                   prefixColor: 'red.dim',
   
               },
               {
                   command: `wait-port localhost:${port} && export NODE_ENV=development PORT=${port} && electron desktop/main.js`,
                   name: 'Main',
                   prefixColor: 'cyan.dim',
               },
           ],
           web: [
               {
                   command: `webpack-dev-server --open --config config/webpack/webpack.dev.js ${args}`,
                   name: 'Webpack Dev Server',
                   prefixColor: 'red.dim',
               },
               {
                   command: 'node web/proxy.js',
                   name: 'Web Proxy',
                   prefixColor: 'cyan.dim',
               },
           ],
           ios: [
               {
                   command: `react-native run-ios ${args}`,
                   name: 'run-ios',
                   prefixColor: '#007AFF',
               },
           ],
           android: [
               {
                   command: `react-native run-android ${args}`,
                   name: 'run-android',
                   prefixColor: '#3DDC84',
               },
           ],
       };
   
       concurrently(platformProcesses[platform], {
           inputStream: process.stdin,
           killOthers: ['failure'],
           prefix: 'name',
       }).then(
           () => process.exit(0),
           () => process.exit(1),
       );
   }).catch(() => process.exit(1));

2. Adjust package.json such that the ios, android, desktop, web scripts point at the start script and pass the respective platform as an argument. Ex: "ios": "node start.js --platform ios".

3. Delete desktop/start.js, because that wouldn't be used anymore.

Thoughts?

[dklymenk]

@roryabraham Sorry, but I do not understand what issue does this script solve.

Is the long term idea here to get the first free port after 8080 (or 8081 for native), and pass it as an argument to the server responsible for packaging for the platform of choice?

I don't think we should do anything like this. Here are some bulletpoints:

• webpack is already doing something similar when you start it, so if you start the three servers in any sequence where metro bundler is not last, for example npm run ios, npm run web, npm run desktop, they will use ports 8081, 8080 and 8082 respectively and there will be no collision thanks to the fact that webpack will notice that port 8081 is in use and start the server on port 8082
• if we are to pass port to react-native run-{android,ios} commands in the sample code above we would run into an issue where every time you run that command it will spawn metro bundler on a new port instead of using bundler you already have, unless we replace portfinder with some custom loop that uses the isPackagerRunning function I showed above
• if it was for me to choose, I would keep the dev experience as vanilla as possible, where any react native dev could open package.json and find familiar stuff

So the only issue with Expensify dev experience my proposal above doesn't solve is the one where if you start web and desktop servers first, the metro bundler won't start because it still wants to use the static port 8081. If I understand the idea behind the code above correctly - use dynamics ports for everything (not just web and desktop servers) - we are accepting the idea that metro bundler won't be always running on port 8081. If that's the case, we can just set metro bundler port to 8082 in the package.json like this

diff --git a/package.json b/package.json
index 8bff03dec..29fde0639 100644
--- a/package.json
+++ b/package.json
@@ -7,12 +7,12 @@
   "license": "MIT",
   "private": true,
   "scripts": {
-    "android": "npm run check-port -- 8081 && react-native run-android",
-    "ios": "npm run check-port -- 8081 && react-native run-ios",
-    "ipad": "npm run check-port -- 8081 && react-native run-ios --simulator=\"iPad Pro (12.9-inch) (4th generation)\"",
-    "ipad-sm": "npm run check-port -- 8081 && react-native run-ios --simulator=\"iPad Pro (9.7-inch)\"",
+    "android": "react-native run-android --port 8082",
+    "ios": "npm run check-port -- 8082 && react-native run-ios --port 8082",
+    "ipad": "npm run check-port -- 8082 && react-native run-ios --port 8082 --simulator=\"iPad Pro (12.9-inch) (4th generation)\"",
+    "ipad-sm": "npm run check-port -- 8082 && react-native run-ios --port 8082 --simulator=\"iPad Pro (9.7-inch)\"",
     "desktop": "node desktop/start.js",
-    "start": "react-native start",
+    "start": "react-native start --port 8082",

Pros:

• unless you are working on some other project and Expensify App at the same time, you will never run into an issue where starting desktop, web, ios, android in any order will break because of port is in use error
• we can even remove the check port script entirely and discard my proposal above as you will no longer run into that error under normal circumstances (we can still leave it, just check for port 8082 this time around)
• no long and complex scripts that might confuse someone starting out on the project
• all the default functionality is still in place:
  • you can start metro bundler separately from npm run android/ios with npm start
  • doing npm run andorid followed up by npm run ios will never spawn a new metro bundler
  • when you rebuild the app, you don't have to kill metro bundler, it will use the existing one

Cons:

• none, if you're ok with metro bundler running on port 8082 all the time

[roryabraham]

@dklymenk thanks for the detailed analysis! I guess I didn't really think about the fact that my solution would spawn a several metro servers on different ports. The reason I gravitated towards the solution I posted in the first place is because it unifies all our app startup scripts into one place. But I think you made some good points about the cons of such a solution:

if it was for me to choose, I would keep the dev experience as vanilla as possible, where any react native dev could open package.json and find familiar stuff

no long and complex scripts that might confuse someone starting out on the project

So after thinking it over some more I think that your latest proposal (using port 8082 for metro) is a great solution. Looking forward to it!

[dklymenk]

Good. So let's confirm the exact change I'm meant to implement with others. There has been a loot of talk, so here is a summary of options we have:

1. My original proposal where I basically delete everything from checkPort script, rename to something like checkMetroBundlerPort (the name is still not set in stone, you can still vote on it) and add the following:

const {isPackagerRunning} = require('@react-native-community/cli-tools');

isPackagerRunning().then((result) => {
    if (result === 'unrecognized') {
        console.error(
            'The port 8081 is currently in use.',
            'You can run `lsof -i :8081` to see which program is using it.\n',
        );
        process.exit(1);
    }
});

2. Just delete that script completely and force usage of port 8082 for metro bundler with these changes to package.json:

diff --git a/package.json b/package.json
index 8bff03dec..29fde0639 100644
--- a/package.json
+++ b/package.json
@@ -7,12 +7,12 @@
   "license": "MIT",
   "private": true,
   "scripts": {
-    "android": "npm run check-port -- 8081 && react-native run-android",
-    "ios": "npm run check-port -- 8081 && react-native run-ios",
-    "ipad": "npm run check-port -- 8081 && react-native run-ios --simulator=\"iPad Pro (12.9-inch) (4th generation)\"",
-    "ipad-sm": "npm run check-port -- 8081 && react-native run-ios --simulator=\"iPad Pro (9.7-inch)\"",
+    "android": "react-native run-android --port 8082",
+    "ios": "react-native run-ios --port 8082",
+    "ipad": "react-native run-ios --port 8082 --simulator=\"iPad Pro (12.9-inch) (4th generation)\"",
+    "ipad-sm": "react-native run-ios --port 8082 --simulator=\"iPad Pro (9.7-inch)\"",
     "desktop": "node desktop/start.js",
-    "start": "react-native start",
+    "start": "react-native start --port 8082",

3. A combination of 1 and 2 where we still force port 8082 but also check if it's good with the new script. We can pass that port to isPackagerRunning function as an arg.

---

Solution 1 Is slightly inferior to 2 because starting webpack for web and desktop will reserve ports 8080 and 8081 causing metro bundler to fail to start.

Solution number 2 does not have such problem. You will never run into an error with ports unless you are working on multiple projects at the same time.

Solution number 3 has all the benefits of 2 but will also yield a fancy error message if metro bundler can't start because something else is running on port 8082.

[HorusGoul]

Could we confirm the --port parameter works correctly with iOS? I remember changing the metro port, but it didn't work with iOS because the 8081 is hardcoded in some iOS-specific files.

[dklymenk]

@HorusGoul
You are right. It is hardcoded: facebook/react-native#9145

The issue is closed, but I can still reproduce it.

Neither the env var RCT_METRO_PORT nor the --port option/flag seem to do the job for me.

So I guess this leaves us with Solution 1?

[dklymenk]

The issue has seen no discussion last week, so I opened a PR with my best take on it.

I have no env setup for windows testing, so it would be nice if someone was to validate the PR on windows.

Thanks.

[tgolen]

None of us have windows environments either, so I think we are OK with just trying out best. Thanks!

[Christinadobrzyn]

PR submitted - under review

[botify]

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[botify]

🚀 Deployed to staging by @marcaaron in version: 1.0.95-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[Christinadobrzyn]

Hold for payment set to [HOLD FOR PAYMENT Sept 17]

[botify]

🚀 Deployed to production by @roryabraham in version: 1.0.96-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[Christinadobrzyn]

7 days since PR deployed - paid @dklymenk in Upwork (including $250 bonus for finding and fixing the bug) and closed the job.