add a serve executor for esbuild

DibyodyutiMondal commented 1 year ago

[x] I'd be willing to implement this feature (contributing guide)

Description

Add a way to serve the output of @nrwl/esbuild:esbuild. A good starting point would be to simply run node . in the output directory, with the env copied from from the nx process, so that .env file support is not broken.

Motivation

Currently, if I use @nrwl/esbuild to build an esm-only node application, then both @nrwl/js and @nrwl/node can't serve it because of require() errors. While the @nrwl/js:node executor looks great, I feel there's a much simpler way to serve a node application when using esbuild.

Sample terminal output for node http server which restarts seamlessly on code changes , with superfast builds by esbuild, and with no other tooling like nodemon, etc, or split terminals, all in a single command

user@device:~/workspace$ nx serve api

> nx run api:serve

[ watch ] build succeeded, watching for changes...
server started...
port: 8088
[ watch ] build succeeded, watching for changes...
server started...
port: 8088

Suggested Implementation

import type { ExecutorContext } from '@nrwl/devkit';
import { esbuildExecutor, EsBuildExecutorOptions } from '@nrwl/esbuild';
import { ChildProcess, spawn } from 'child_process';

export default async function* customEsbuildServeExecutor(
  options: EsBuildExecutorOptions,
  context: ExecutorContext
) {
  // force watch mode to make this work
  options.watch = true;

  let existingCp: ChildProcess | undefined;
  for await (const result of esbuildExecutor(options, context)) {

    // wait to close previous running process if exists
    await new Promise<void>(resolve => {
      if (existingCp) {
        existingCp.on('close', () => resolve());
        existingCp.kill();
      }
      else
        resolve();
    });

    const cp = spawn(
      'node', ['.'],    //  execute `node .`.
      {
        cwd: options.outputPath,  //  execute in the output directory
        env: process.env,  //  pass in the env from the nx process
        stdio: 'inherit' // so that `console.log`, `console.debug`, in the app work as expected
      });
    existingCp = cp;
    // clear the variable in case the process exits early (like with errors)
    cp.on('close', () => existingCp = undefined);

    yield result;
  }
}

Possible improvements

ability to pass a custom command and not just 'node .'
ability to pass in custom arguments
edit the schema since 'watch' must be true for this to work
instead of adding a separate executor, we can just add a --serve flag which progressively adds this feature
- nx build api for normal builds.
- nx build api --watch for builds with normal watch mode.
- nx build api --serve to build once and serve the node application.
- nx build api --watch --serve to build, watch and serve the node application with changes.

EDIT (07-02-2023):

the child process would not restart if it ended with an error on startup. made changes to the executor to fix that

shailesh-ladumor commented 1 year ago

is --serve option available for the angular app?

DibyodyutiMondal commented 1 year ago

The executor I am recommending is specifically for node applications. The current easiest method to serve an api in node.js with @nrwl/esbuild is to use watch mode with nodemon. This feature request hopes to eliminate the need for nodemon as a dependency, and provide a single command to do it.

As far as angular goes, I believe the angular dev server forces the use webpack. Even if we specify the esbuild executor for builds, it'll still fall back to webpack for serving. I think, though, that if we play around with the serve options provided by esbuild itself, we can use that as a replacement for the entire angular dev server executor, minus hot reloading capabilities for javascript. esbuild currently only supports hot-reloading for css.

I tried it once long ago, but at that time, I ran into problems with scss, so I continued with webpack. Additionally, I found that webpack is easier to work with than esbuild if we want a custom service-worker with angular. (I use workbox instead of angular's service worker). Hopefully all that will change in the future.

veimox commented 1 year ago

FYI and as far as I know, this could be solved by https://github.com/nrwl/nx/pull/10414 but it was closed by @nartc in September 2022 with a much awaited 'better solution'.

Could this style be a better solution @nartc?

DibyodyutiMondal commented 1 year ago

I checked out version 15.7 and tried combinations of

building - @nrwl/esbuild:esbuild
serving - @nrwl/js:node, @nrwl/node:node

Both fail with:

workspace-root/node_modules/.pnpm/@nrwl+js@15.7.2_wi7kwhfbxlhncdveqfqfnunpki/node_modules/@nrwl/js/src/executors/node/node-with-require-overrides.js:16
        return originalLoader.apply(this, arguments);
                              ^
Error [ERR_REQUIRE_ESM]: require() of ES Module workspace-root/dist/apps/api/index.js from workspace-root/node_modules/.pnpm/@nrwl+js@15.7.2_wi7kwhfbxlhncdveqfqfnunpki/node_modules/@nrwl/js/src/executors/node/node-with-require-overrides.js not supported.

I still have to use custom executors to serve assets properly and run the node server properly. I am not even sure that @nrwl/esbuild is the right package to implement this anymore. Maybe @nrwl/js:node or @nrwl/node:node are better places to put this code into.

The support for node via --preset node-server seems like a step in the right direction, but only for commonjs. I have seen multiple open and closed issues about the lack of esm support, and all of them seem to point to the fact that dynamically choosing import/require as per the application is not working as intended.

It certainly merits investigating why it's not working, but in the meantime, why not use a child process with node . as shown above? After all - it works. node . will always make the correct choice by looking at the dist/application/package.json and the executor doesn't have to do anything except orchestrating the child process lifetime with build emits from code changes.

mikehaas763 commented 1 year ago

When I get this error there's also a message: Instead change the require of main.js in node_modules/@nrwl/js/src/executors/node/node-with-require-overrides.js to a dynamic import() which is available in all CommonJS modules.. Maybe a PR to change to import() is the easiest way to handle this? If Nx expects that we support older versions of Node where import was not available, we could do some environment detection to figure out which to do

nartc commented 1 year ago

Hi folks, I understand the frustration regarding ESM. However, this is a little bit involved and I'm here to give you some more context from Nx side of things.

The Nx executors (eg: nrwl/js:node) help with resolving Path Alias. As you already know, using libs in Nx workspace is common and we'd like to keep the experience consistent when using our plugins. To support serve with path aliases, we override the require in the compiled code so that path alias still works. With that, we're currently tied with CommonJS for now.

If you have a custom executor that works for you in terms of ESM, please continue to use that. In the mean time, we'll continue looking into ESM support without introducing breaking change or the least amount of breaking changes.

DibyodyutiMondal commented 1 year ago

@nartc If I may go a tangent here.

Let's distinguish between 'serve' executors and 'build' executors. Ex: @nrwl/esbuild:esbuild is a build executor. @nrwl/node:node is a serve executor. Also, per my experience, serve executors must be passed a buildTarget option - which is a target that uses a build executor.

The Nx executors (eg: nrwl/js:node) help with resolving Path Alias. As you already know, using libs in Nx workspace is common and we'd like to keep the experience consistent when using our plugins.

I do not follow this part. As far as I can tell, resolving path aliases is already being handled by the 'build' executor, which is then passed to the 'serve' executor via 'buildTarget'.

Besides, when we generate applications in newer versions of nx, we get @nrwl/esbuild:esbuild as a build executor and @nrwl/node:node as a serve executor by default.

Now, since esbuild is a bundler, we don't have to worry about path aliases because it's all bundled. Otherwise, using the node . command on the output would not work.

I suppose, the question we should ask ourselves is, why should a serve executor resolve path aliases? A build executor has to resolve path aliases anyway in order to build a project. And all of the serve executors I have come across must be provided a 'buildTarget' as per the schema. If you think about it - we're doing the same thing twice here.

Imagine if you decided to not make serve executors resolve path aliases, and simply used the build output as is - would that be a good thing to do? I am sure there are cases where the serve target must do the resolving by itself; but when we are already using a powerful bundler, the serve executor can (and should) relax. Maybe an if-check can solve our problems here?

Or, since this idea is something that specifically depends on @nrwl/esbuild:esbuild, instead of making changes to @nrwl/js:node or @nrwl/node:node, we should use a new executor @nrwl/esbuild:serve to clearly indicate that this serve executor depends on @nrwl/esbuild:esbuild for the buildTarget.

PS: From this monologue itself, it's evident there are quite a few things to consider here. As you suggested, I shall continue to use the custom executor I am already using. I shall put the code in this thread while we wait.

nartc commented 1 year ago

I do not follow this part. As far as I can tell, resolving path aliases is already being handled by the 'build' executor, which is then passed to the 'serve' executor via 'buildTarget'.

It depends between bundling vs compiling (I haven't had the chance to play with esbuild:esbuild since I haven't had the capacity to work on Nx until now, so my experience with it is not good). Even so, bundling with workspace libraries set as external also is a different case.

In those cases where the libraries' code are not bundled and still are import {...} from '@my-workspace/my-lib' (instead of bundled or inlining) then the serve executor has to handle the path mappings. That said, there's not an easy way for the serve executor to be generic enough and detect what's what so the code for handling path mappings is always there (eg: nrwl/js:node)

Thank you for all the suggestions, I'm literally taking notes here. ESM/CJS as well as path mappings are tricky problems to solve in a Monorepo setting. Now that I have more capacity, I'll start to work towards these goals more actively.

Thanks again!

veimox commented 1 year ago

I am happy to see this mental model that separates the different operations.

Given that serve should serve most of use cases in a generic way the Nx way to tackle this would be to expose the capabilities as a config flag. In the light of that I can see how we were reluctant to the proposal made on #10414. What do you think about a flag such transpileLocalModules or something like that such we use that as an entry point for either 'flat node' or 'monkey patching node'?

Sorry that I jumped into this issue btw, I just think that is a very wanted/needed feature for the node users and it is a bit contradictory that we are missing this whilst we claim to have First-Class support for Node. I think we are very close and we will be there once we jump into the Node 2020 ESM suport.

DibyodyutiMondal commented 1 year ago

That said, there's not an easy way for the serve executor to be generic enough and detect what's what

@nartc Indeed, it is difficult for a serve executor to see what's what. In one of my early experiments to see whether I can make an executor that would look at the build target, executor, options and make decisions based on that. It started becoming waaay too complicated waaay too fast, and I wasn't even doing anything fancy. It would be a nightmare to maintain and improve.

I thought to myself, surely there must be an easier way to do this.

For the moment, let's forget about a serve executor and set a simple goal that we want to achieve manually:

we want to use @nrwl/esbuild:esbuild to build a node app.
we want to serve the node app that was built.

Note that we are being very specific with our build executor, because that's what this issue is targeted towards.

When we say 'serve the app', what we really mean is 'execute the built package with node'. It does not matter if it's a script or a cli app or an http server - when people want to serve any node app, what they really do is simply execute it.

Ultimately, our serve executor will simply automate this manual process.

What are the requirements to make a node app executable?

package.json must correctly specify, the main, module and type fields
all imports/requires must be 'visible' to node

When we run the @nrwl/esbuild:esbuild executor, it does a few things:

it ensures that any library projects the app depends on are built. The output of those libraries will then be present in their respective dist locations. It doesn't matter if they're marked as external, because esbuild is not in the picture yet - this is core nx at work
it builds and bundles the app using esbuild
it sets the main, module and type fields correctly in the output package.json
if any import was not bundled, it'll add it to the dependencies and/or peerDependencies field, as per the configuration passed to the executor. Per my experience - these will include only the following types of packages (correct me if I am wrong)
- any library projects that were marked as external, and
- npm packages that were not bundled, either by external, or by setting thirdParty to false

Obviously, we don't need to worry about any bundled imports because esbuild has handled it for us. As for the unbundled imports, esbuild has already created a list that it expects to be available at runtime. Therefore, the only thing left to do is to meet that expectaion.

Given all this information, we can come up with concrete steps that we can do manually to achieve the simple goal set out above:

Run the build target as-is
Get the list of dependencies + peerDependencies from the output package.json
Npm packages will already be available in workspace-level node_modules or build would not have succeeded, so filter this list and take only the packages which are library projects in the workspace.
Library projects will have their output in their respective dist location. Edit the dependency in package.json to use a file:... notation. Use the relative path from the output pacakge.json to the dist location of the library in question
- Alternatively, create a node_modules folder in the app's dist folder, and symlink the library's dist folder to a sub-folder of this new node_modules folder. Ensure the sub-folder name/path matches the library's import.
Alternatively, we can symlink dist/libs to dist/node_modules. This will make all the project library outputs available to any node program running inside the dist folder. This will also eliminate steps 2-4 completely, and make things so much simpler.
- This is probably a much, much better option because what happens if a library project depends on another project? I never mark project libraries as external, so I never got to this point, so I don't know for sure if it'd be better.
Run the package with the node command

Optimization: Steps 2-5 are unnecessary if people do not have project libraries that they mark as external. I can't think of why someone would use a bundler like esbuild and then not bundle certain parts of their code. Maybe build size and performance issues? It'd be odd if those happened with esbuild, but I guess they definitely can. Bottom line is that I expect that a majority of users would not have their libraries marked as external, but that's just my personal opinion. So if you want, we can have a step between 1 and 2: 1.5. Check if the external field was provided in the build target options. If it was provided continue, else stop.

Now, all a serve executor needs to do is to run the executor in watch mode, carry out the steps and manage the lifecycle of the child process, i.e, re-run it when a code change causes a build emit

From all this, we can see, the only real blocking problem is that built libraries will not be 'visible' to node by default. Nx tries to solve this problem by patching require/import to resolve the dependencies properly. It works, but so far only with commonjs. Using symlinks is simpler, and it allows node to do things natively. In a way, it's 'closer' to how the app would run in production.

But since no one did it till now, I'm thinking maybe there are caveats that I am unaware of?

Also, if you think about it, this is similar to what one would do for production deploys. If a project library is marked as external -they'd have to upload the library output somehow into either node_modules or use a file:.. dependency to another folder.

nartc commented 1 year ago

@DibyodyutiMondal Thank you for the detailed response. I did read it couple of times and it seems the setup is pretty specific to your workspace. If anything, I'd think that a nrwl/esbuild:dev-server is needed instead of trying to put all of these logic in nrwl/js:node.

We're careful about adding new serve executors though so I'll discuss with my peers about this and get back to you later.

DibyodyutiMondal commented 1 year ago

I see. I wish my approach could be more generalized. Then it'd be of greater help to others as well.

At any rate, a dev server for esbuild is what this issue is asking for, after all.

nartc commented 1 year ago

I'll be closing this as there's no action item on our end but we're happy to receive PRs regarding this ESBuild dev server if it can be made generic enough.

DibyodyutiMondal commented 1 year ago

Awesome! I shall try to create a PR as soon as I can. But will it possible to provide a list of conditions that this executor must satisfy to be considered generic enough. Then I can write the logic to fulfill those conditions.

nartc commented 1 year ago

@DibyodyutiMondal Thanks! Sure, off top of my head, default esbuild:esbuild app, bundle:false, different format: [...], and maybe try external: [...]

DibyodyutiMondal commented 1 year ago

@nartc I created this very minimal demo repository, to show the executor in action. https://github.com/DibyodyutiMondal/custom-nx

If possible, could you take a look and give some feedback? There is a node application api and a library utils. Feel free to change the build target options of both the library and application - trying out different combinations of 'bundle', 'external' and 'format' as you feel necessary.

eskres commented 1 year ago

@nartc @DibyodyutiMondal Any luck with a PR? The require() error from node-with-require-overrides.js is killing me!

DibyodyutiMondal commented 1 year ago

I was given a list of conditions to satisfy before I can submit a PR, so I made a repository with the custom builder. I haven't heard back from @nartc about whether it's viable enough for a PR.

You could check it out and give your feedback as well.

DibyodyutiMondal commented 1 year ago

In fact, it'd be great if more people from different ecosystems checked out the repo, added some minimum code and see if it can build and serve as expected. That'd give a better picture of whether the builder can stand up to community requirements.

github-actions[bot] commented 1 year ago

This issue has been closed for more than 30 days. If this issue is still occuring, please open a new issue with more recent context.

nrwl / nx