feat: suppport for NX monorepos #387

FinnDore · 2022-08-07T12:13:43Z

What does this PR address?

Out of the box support for NX mono repos. ( nexjs apps, express apps and apps that build to an index.js )

Make sure to follow GitHub's guide on creating PR.
Did you read through contribution guidelines?
Did your changes require updates to the documentation? Have you updated those accordingly? - New env var so yes?
Did you write tests to cover your changes? Did it pass locally when running cargo test?
Have you run cargo fmt, cargo check and cargo clippy locally to ensure that CI passes?

src/providers/node.rs

FinnDore · 2022-08-07T20:33:51Z

Think this is ready for a review whenever someone is free 😊

…packs into finndore/nixpack-nx

examples/node-nx/.dockerignore

examples/node-nx/.editorconfig

src/providers/node.rs

coffee-cup · 2022-08-08T18:39:40Z

src/providers/node.rs

+            ));
+            // Remove the package.json so when we run node /dist/**/file.js node does
+            // not look at the package.json and infer things it should not, Such as wether its a es module or not.
+            build_phase.add_cmd("rm /app/package.json".to_owned());


I am a little confused by this. It is not common to remove the package.json file. Why is it necessary in this case? I've run many NX apps without the need for removing this file. I also know a few apps that rely on this app existing in production.

This also won't work if the start command is overriden to be something like npm run web:start

The reason I chose not to use the serve target is that it appears to be for development purposes as it includes a websocket for use with a debugger and hot-reload on file change. If this is ok we can totally use nx run <app-name>:serve:production see the output below. The other three targets that are generated by the default express generator (@nrwl/express:aplication) are lint, build and test.

npx nx run express-app:serve:production > nx run express-app:serve:production chunk (runtime: main) main.js (main) 564 bytes [entry] [rendered] webpack compiled successfully (02f31e3162b7080a) Debugger listening on ws://localhost:9229/666612ae-1046-4d7f-aefd-f39ef9b3fc16 Debugger listening on ws://localhost:9229/666612ae-1046-4d7f-aefd-f39ef9b3fc16 For help, see: https://nodejs.org/en/docs/inspector nx express app works chunk (runtime: main) main.js (main) 572 bytes [entry] [rendered] webpack compiled successfully (4275d6c19665a755) Command failed: taskkill /pid 28756 /T /F ERROR: The process "28756" not found. Debugger listening on ws://localhost:9229/f9432e74-f926-4596-8ad1-a6a5175ef466 Debugger listening on ws://localhost:9229/f9432e74-f926-4596-8ad1-a6a5175ef466 For help, see: https://nodejs.org/en/docs/inspector nx express app works, reload

( same story with @nrwl/node:webpack )

The reason for deleting the package.json was to solve an issue where running node <path-to-app>/main.js would have some interaction with the package.json. This iirc. Maybe it would be better ( if we don't use the serve target ) to cd into the output folder and run node main.js?

ReferenceError: require is not defined in ES module scope, you can use import instead This file is being treated as an ES module because it has a '.js' file extension and '/app/package.json' contains "type": "module". To treat it as a CommonJS script, rename it to use the '.cjs' file extension.

I agree it doesn't make sense to use the serve target. But lots of apps have another separate start task/script that is meant for production. If that target is found we can run that command. As for the ES module error, I think that can be solved with a field in the package.json file. If the app was deployed and run in other production environments it would not be common to delete the package.json file.

Unfortunately deleting this file is not really feasible with Nixpacks since some users may override the start command with the NIXPACKS_START_CMD variable to something like npm run start. This should work even for NX apps.

Ah! i understand now, we check if they have a start target and if they don't we fall back. I'm thinking we do the checks in this order: <appName>:start:production -> <appName>:start -> node <mainFile>.js -> node index.js.

Regarding the package.json thing, I think your right, I tried that but it also felt a bit weird. Instead, we could cd into the dist dir ( either cd dist or cd <outputdir> ) and run the fallback start command. Whats is your thoughts?

+1, I agree that start command priority order makes sense.

As for editing package.json, I think it is fairly common to do when running generated code in a production environment. I must admit, I haven't written raw JavaScript in a long time (mainly just TS), but changing the module type in package.json should be okay. The main thing is that we should not be manually deleting these files for the user just to run their code.

That's fair, ill get on that tomorrow (hopefully) when I have time, serde should make this easy. Thanks for the insights as well.

Just removed the rm package.json step and ran the tests ran fine, so I think I've been lying about the module-type issue.
Ill make the start command change and then I can asses if its really an issue

coffee-cup · 2022-08-08T18:45:16Z

src/providers/node.rs

+            let project_json = NodeProvider::get_nx_project_json_for_app(app, env)?;
+
+            let executor = project_json.targets.build.executor;
+            if executor == "@nrwl/next:build" {


A few comments for the NX start command

The port shouldn't be set when running a Next app. It should just use the default

Instead of cd'ing into the directory, I think it is cleaner to start the app with Nx. E.g. nx run frontend:start

The default start command for all NX apps should be running the start command in the app (if it exists). For example for something like Yarn/NX monorepos, each sub-app will have it's own start command that we should use (if it exists). Then we can fallback to running dist/main.js

Same as the other start commands.

Regarding the port. The default port is something like 3200, I would expect a website to be hosted on port 80 and without this change, it would not be. removing this the user would have to overnight the start command manually to use port 80.

Would the default port be acceptable?

For Next apps the port is overridable with the PORT environment variable and that is common in many other frameworks. Also, when starting the container, you can map port 3200 inside of the container to any other port on the outside, so the default port in the container does not matter.

Sounds good to me. I'll get on this change when I can.

Should be done now. Just the run commands to change as of present.

src/providers/node.rs

tests/docker_run_tests.rs

src/providers/node.rs

JakeCooper · 2022-08-11T20:24:57Z

As a result of this provider, can we revert #382?

The reason we added it was for the nwl stuff

FinnDore · 2022-08-11T20:28:17Z

Tbh I have never needed this env var on Railway. Having said that @ChristianIvicevic said not having it causes a sigTerm? I have noticed a sig term on vercel before when using nx but it's very flakey and may be caused by me not setting this env var.

ChristianIvicevic · 2022-08-11T20:30:43Z

CI=true makes a lot of sense with Node projects such as disabling watch modes for tools (such as tests) or in the case of Nx disabling the background daemon that caused SIGTERMs.

coffee-cup · 2022-08-11T22:53:19Z

As a result of this provider, can we revert #382?

The reason we added it was for the nwl stuff

This PR does not change the fact that CI=true is needed. It just adds better first class support for NX apps.

docs/pages/docs/providers/node.md

coffee-cup · 2022-08-11T23:10:03Z

src/providers/node/mod.rs

-            let pkg_manager = NodeProvider::get_package_manager(app);
+        if NodeProvider::is_nx_monorepo(app) {
+            let app_name = NodeProvider::get_nx_app_name(app, _env)?.unwrap();
+            build_phase.add_cmd(format!("{} run {}:build:production", pkg_manager, app_name));


Should this be npm nx run APP:build:production since this script may not exist. I am seeing this error when building the default NX app.

I believe you are correct. I'm refactoring the build command so it works no matter what package manager the user is using. Just didn't get enough time last night 😆

…/nixpack-nx

FinnDore · 2022-08-12T23:13:19Z

Think all the ducks are in order now. This should be ready for a re-review whenever you are free next week or so 😀.

coffee-cup

It's looking good! Can you please fix the linting errors and then I will merge :D

FinnDore · 2022-08-16T18:52:37Z

Done!

FinnDore · 2022-08-16T23:14:30Z

Odd, somehow I missed some. I'll get on that asap tomorrow.

coffee-cup · 2022-08-17T03:47:53Z

I just went ahead and fixed the remaining lint errors (Literally just had to run cargo clippy --fix) 😄

FinnDore added 7 commits August 3, 2022 19:39

chore: added an example nx project

191c51b

feat: added changes to support nx

42e2bc1

chore: added some more fixes for express apps

6ec3a98

chore: some fixes to make commonjs files run

38d5772

chore: some cleanup

fef09d6

chore: added a help full comment :)

8e1e235

chore: added a fallback of dist/apps/<app_name> for get output path

fa724e8

FinnDore changed the title ~~Finndore/nixpack nx~~ feat: suppport for NX monorepos Aug 7, 2022

FinnDore mentioned this pull request Aug 7, 2022

First-class support for nrwl/nx and applications within such a monorepo #374

Closed

FinnDore added 4 commits August 7, 2022 18:31

test: fixed nx next test

caa2c52

test: added a test for a node-app

81ff864

chore: changed the nextjs port from 3000 to 80

5db7daa

chore: fixed clippy issues 📎

2500a3d

FinnDore commented Aug 7, 2022

View reviewed changes

src/providers/node.rs Outdated Show resolved Hide resolved

docs: some additions to the docs

3a0c622

FinnDore marked this pull request as ready for review August 7, 2022 20:33

FinnDore added 3 commits August 8, 2022 18:23

Merge branch 'railwayapp:main' into finndore/nixpack-nx

e7a1712

chore: removed some stray logging that i added

8fa2798

Merge branch 'finndore/nixpack-nx' of https://github.com/FinnDore/nix…

1f3d445

…packs into finndore/nixpack-nx

coffee-cup reviewed Aug 8, 2022

View reviewed changes

FinnDore added 2 commits August 8, 2022 21:46

chore: actioned some review feedback

279189b

lint: fixed lint waning

cd23611

coffee-cup reviewed Aug 8, 2022

View reviewed changes

tests/docker_run_tests.rs Outdated Show resolved Hide resolved

FinnDore added 3 commits August 9, 2022 17:37

chore: removed the port arg from the next nx start command

06b203c

chore: removed unneeded networks

fcf0873

chore: prevented each nx test building its image twice

b0153c5

coffee-cup reviewed Aug 10, 2022

View reviewed changes

src/providers/node.rs Outdated Show resolved Hide resolved

FinnDore added 2 commits August 10, 2022 15:26

Merge branch 'railwayapp:main' into finndore/nixpack-nx

2e849fa

chore: moved nx structs to their own file

b50f616

FinnDore added 4 commits August 10, 2022 21:54

chore: pub 🍻

c125304

chore: removed the rm command from the build step

2a49a08

chore: updated start commands to use potential start target

c29f5d7

chore: removed unneeded default

ab415db

FinnDore added 3 commits August 11, 2022 21:50

chore: added two new projects and support for start commands

3411184

chore: re-ordered start commands

eb76922

chore: build command change

84602d9

coffee-cup reviewed Aug 11, 2022

View reviewed changes

FinnDore added 8 commits August 12, 2022 13:18

chore: reverted changes to build command

f0ccaae

chore: removed unneeded process.exit's

780dd68

chore: moved to npx to run nx commands

4d44f8b

chore: removed default no need fot it

30bbda3

docs: updated start commands to include new options

aec5625

chore: clippy fixes

5e86bb1

Merge commit '5e860d9bfcaff721673fc2aef23914c9a3569c37' into finndore…

0fcdf71

…/nixpack-nx

chore: fixed merge issues

c6515fa

coffee-cup approved these changes Aug 16, 2022

View reviewed changes

fixed: 📎

a0ea70a

coffee-cup added 3 commits August 16, 2022 20:37

Merge branch 'main' into finndore/nixpack-nx

b709d46

lint fixes

f9e08bc

add nx generate plan test snapshot

f1904f6

coffee-cup merged commit 91b7363 into railwayapp:main Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: suppport for NX monorepos #387

feat: suppport for NX monorepos #387

FinnDore commented Aug 7, 2022 •

edited

Loading

FinnDore commented Aug 7, 2022

coffee-cup Aug 8, 2022

FinnDore Aug 8, 2022

coffee-cup Aug 8, 2022

FinnDore Aug 9, 2022

coffee-cup Aug 9, 2022

FinnDore Aug 9, 2022

FinnDore Aug 11, 2022

coffee-cup Aug 8, 2022

FinnDore Aug 8, 2022

coffee-cup Aug 8, 2022

FinnDore Aug 9, 2022

FinnDore Aug 9, 2022

JakeCooper commented Aug 11, 2022 •

edited

Loading

FinnDore commented Aug 11, 2022

ChristianIvicevic commented Aug 11, 2022

coffee-cup commented Aug 11, 2022

coffee-cup Aug 11, 2022

FinnDore Aug 12, 2022

FinnDore commented Aug 12, 2022

coffee-cup left a comment

FinnDore commented Aug 16, 2022

FinnDore commented Aug 16, 2022

coffee-cup commented Aug 17, 2022

feat: suppport for NX monorepos #387

feat: suppport for NX monorepos #387

Conversation

FinnDore commented Aug 7, 2022 • edited Loading

What does this PR address?

FinnDore commented Aug 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeCooper commented Aug 11, 2022 • edited Loading

FinnDore commented Aug 11, 2022

ChristianIvicevic commented Aug 11, 2022

coffee-cup commented Aug 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FinnDore commented Aug 12, 2022

coffee-cup left a comment

Choose a reason for hiding this comment

FinnDore commented Aug 16, 2022

FinnDore commented Aug 16, 2022

coffee-cup commented Aug 17, 2022

FinnDore commented Aug 7, 2022 •

edited

Loading

JakeCooper commented Aug 11, 2022 •

edited

Loading