web-infra-dev / rspack

The fast Rust-based web bundler with webpack-compatible API 🦀️
https://rspack.dev
MIT License
9.37k stars 543 forks source link

External dependencies being set as web instead node target #2741

Closed dgmachado closed 1 year ago

dgmachado commented 1 year ago

System Info

System: OS: macOS 13.1 CPU: (10) arm64 Apple M1 Pro Memory 32GB Shell: /bin/zsh

Binaries: node: 16.13.2 npm: 9.5.0 yarn: 1.22.19

Browsers: Chrome: 111.0.5563.146

npmPackages: @nrwl/rspack: 15.9.5 @rspack/core: ~0.1.0 @rspack/dev-server: ~0.1.0 @rspack/plugin-minify: ~0.1.0

Details

I'm working on an Nest.js app that has many external dependencies. Setting them all on the array "config.externals" works well (build and serve calls work). However, we'd like to avoid it because we want to bundle all deps inside the deployed package. So on my first try, I removed the config.externals customization and I got build error: few missing deps. After installing the missing deps I got the building working well despite getting the below serve error:

/jose/dist/browser/runtime/webcrypto.js:1
export default crypto;
               ^
ReferenceError: crypto is not defined
    at Object../node_modules/jose/dist/browser/runtime/webcrypto.js (../.../..//node_modules/jose/dist/browser/runtime/webcrypto.js:1:16)

I found it curious that in the above scenario (not customizing config.externals) the node_modules/jose/dist/browser/runtime/webcrypto.js is being used however when I customize the config.externals (success scenario) node_modules/jose/dist/node/cjs/runtime/webcrypto.js is being used. So when we aren't setting the config.externals seem it's bundling some deps as web instead node. What do you think about this? How can I avoid this issue?

Reproduce link

No response

Reproduce Steps

  1. Create a Nest.js app
  2. Use many different external dependencies in your project
  3. Don't set the array config.externals with your external dependencies
  4. It builds with success
  5. You can reproduce the issue trying to serve the generated bundle.

Obs: In my scenario I'm using the below dependencies:

  config.externals = {
    '@nestjs/common': '@nestjs/common',
    '@nestjs/core': '@nestjs/core',
    'tslib': 'tslib',
    "@aws-sdk/util-user-agent-node": "@aws-sdk/util-user-agent-node",
    "@nestjs/mapped-types": "@nestjs/mapped-types",
    "@nestjs/swagger": "@nestjs/swagger",
    "@nestjs/platform-express": "@nestjs/platform-express",
    "@newrelic/pino-enricher": "@newrelic/pino-enricher",
    "class-transformer": "class-transformer",
    "class-validator": "class-validator",
    "engine.io": "engine.io",
    "express": "express",
    "mongodb": "mongodb",
    "newrelic": "newrelic",
    "pino": "pino",
    "pino-http": "pino-http",
    "rest-facade": "rest-facade"
  };
hardfist commented 1 year ago

it seems you import the wrong result of jose, it has both node and browser entry, you should import the node entry. maybe you forget to set target='node' in your configuration, we have a nestjs example for reference here https://github.com/web-infra-dev/rspack/tree/main/examples/nestjs

dgmachado commented 1 year ago

I'm setting the below options on my rspack.config.js file and still doesn't get the correct dep: config.externalsType = 'commonjs'; config.target = "node";

hardfist commented 1 year ago

can you provide minimal demo?

dgmachado commented 1 year ago

@hardfist In working in create a minimal demo reproducing this issue, meanwhile, I've created a Nest.js+rspack app https://github.com/dgmachado/NestJS-RESTful-API (commands: npm run build and npm run serve) and it's failing the build if I don't set the externals option.

I'd like to comment out the below block and let the rspack auto infer the external dependencies and bundle the deps into our main.js file. Is it possible?

    externals: {
      '@nestjs/common': '@nestjs/common',
      '@nestjs/core': '@nestjs/core',
      '@nestjs/swagger': '@nestjs/swagger',
      'tslib': 'tslib',
    },

Even if it's necessary we explicitly set the externals is it possible config to bundle the deps into our main.js file or dist folder?

hardfist commented 1 year ago

have you tried bundle your app by webpack, if it works in webpack then we should make it work in rspack. I tried bundle your app using webpack, and it generates following errors image

so i think it's a question about how to bundle nestjs using webpack|rspack, and it seems nestjs have some peerDep external problems that you need to deal it manually

dgmachado commented 1 year ago

I've created another sample to validate the unique bundle using the webpack (https://github.com/dgmachado/bundled-nest). I don't know how to convert this to use rspack. It was necessary to customize the IgnorePlugin and I just saw the Rspack doesn't support the IgnorePlugin (webpack), so just created a new github issue https://github.com/web-infra-dev/rspack/issues/2788.

hardfist commented 1 year ago

I think this could be implemented when external function is supported, related #2277

hardfist commented 1 year ago

@dgmachado you can use external to auto handle nestjs bundle now

  externals: [function(obj, callback){
    const resource = obj.request;
     const lazyImports = [
          '@nestjs/core',
          '@nestjs/microservices',
          '@nestjs/platform-express',
          'cache-manager',
          'class-validator',
          'class-transformer',
          // ADD THIS
         '@nestjs/microservices/microservices-module',
         '@nestjs/websockets',
         'socket.io-adapter',
         'utf-8-validate',
         'bufferutil',
         'kerberos',
         '@mongodb-js/zstd',
         'snappy',
         '@aws-sdk/credential-providers',
         'mongodb-client-encryption',
         '@nestjs/websockets/socket-module',
         'bson-ext',
         'snappy/package.json',
         'aws4'
        ];
    if(!lazyImports.includes(resource)){
      return callback();
    }
    console.log('resource:', resource);
    try {
      require.resolve(resource);
    }catch(err){
      callback(null, resource);
    }
    callback();
  }],
hardfist commented 1 year ago

@dgmachado you can check the example here https://github.com/web-infra-dev/rspack/blob/main/examples/nestjs/rspack.config.js#L12

hardfist commented 1 year ago

closed as resolved, feel free to open if you still met problems.

dgmachado commented 1 year ago

@hardfist following your example https://github.com/web-infra-dev/rspack/blob/main/examples/nestjs/rspack.config.js#L12 I could make it work in a few examples but I've created an example using MongoDB that it doesn't work. Please take a look at https://github.com/dgmachado/bundled-nest and you can execute the below commands:

npm run build:rspack
npm run start:rspack

Issue:

[Nest] 90438  - 04/25/2023, 12:03:57 PM     LOG [NestFactory] Starting Nest application...
[Nest] 90438  - 04/25/2023, 12:03:57 PM     LOG [s] s dependencies initialized +5ms
[Nest] 90438  - 04/25/2023, 12:03:57 PM   ERROR [o] n.connect is not a function
TypeError: n.connect is not a function
    at f.useFactory [as metatype] (..../14-mongoose-base/dist/main.js:60:1818895)
hardfist commented 1 year ago

@dgmachado it's a known bug about namespace interop which is tracked here https://github.com/web-infra-dev/rspack/issues/2702, so you can just change import * as mongoose from 'mongoose' to import mongoose from 'mongoose' which will work

dgmachado commented 1 year ago

@hardfist That's great this example works. Thank you. I have another private example with a different error. Do you have any suggestions?

Issue:

error[javascript]: JavaScript parsing error
  ┌─ node_modules/node-gyp/lib/Find-VisualStudio.cs:9:7
  │
9 │ using System;
  │       ^^^^^^ Expected ';', '}' or <eof>

To make the build pass was necessary use the below lazyImports collection:

const lazyImports = [
        "@nestjs/core",
        "@nestjs/microservices",
        "@nestjs/platform-express",
        "cache-manager",
        "class-validator",
        "class-transformer",
        "utf-8-validate",
        "bufferutil",
        "class-transformer/storage",
        "@nestjs",
        "@nestjs/microservices/microservices-module",
        "npm",
]

Obs: This example uses -> require('newrelic')

hardfist commented 1 year ago
error[javascript]: JavaScript parsing error
  ┌─ node_modules/node-gyp/lib/Find-VisualStudio.cs:9:7
  │
9 │ using System;
  │       ^^^^^^ Expected ';', '}' or <eof>

this seems c# code? why you bundle c# code in javascript application?

dgmachado commented 1 year ago

Yes seems c#. I'm not bundling this intentionally. If I remove the "npm" from the above lazyImports collection I get the below errors:

imported by ../src/app/modules/calendar-mappings/calendar-mappings.controller.ts

error[internal]: Resolve error
    ┌─ node_modules/@newrelic/native-metrics/lib/pre-build.js:121:24
    │
121 │     const npmPkgPath = require.resolve('npm')
    │                        ^^^^^^^^^^^^^^^^^^^^^^ Failed to resolve npm in /Users/douglasmachado/Documents/Rosa/rosa/node_modules/@newrelic/native-metrics/lib/pre-build.js

error[javascript]: JavaScript parsing error
  ┌─ node_modules/node-gyp/lib/Find-VisualStudio.cs:9:7
  │
9 │ using System;
  │       ^^^^^^ Expected ';', '}' or <eof>

It's very weird, the npm and c# shouldn't be imported.

It's just a simple Nest.js application that before I use Rspack the app was using Webpack with the below external dependencies:

        "externalDependencies": [
          "@nestjs/common",
          "@nestjs/core",
          "@nestjs/mapped-types",
          "@nestjs/platform-express",
          "@newrelic/pino-enricher",
          "pino",
          "pino-http",
          "class-validator",
          "newrelic"
        ],

I've replaced Webpack with Rspack to improve the build in addition to trying to generate a bundle including all external dependencies.

dgmachado commented 1 year ago

I could fix this issues it was necessary to make the below config:

  config.externals = [
        function (obj, callback) {
            const resource = obj.request;
            const lazyImports = [
        "@nestjs/core",
        "@nestjs/microservices",
        "@nestjs/platform-express",
        "@nestjs",
        "@nestjs/microservices/microservices-module",
        "class-transformer",
        "class-validator",
        "engine.io",
        "express",
        "mongodb",
        "pino",
        "pino-http",
        "kerberos",
    "snappy",
    "bson-ext",
    "snappy/package.json",
    "@mongodb-js/zstd",
    "mongodb-client-encryption",
    "bufferutil",
        "class-transformer/storage",
        "utf-8-validate",
        "aws-crt"
];
      if (resource === "newrelic" || resource === "rest-facade") {
        require.resolve(resource);
        callback(null, resource);
        return;
      }
            if (!lazyImports.includes(resource)) {
                return callback();
            }
            try {
                require.resolve(resource);
            } catch (err) {
                callback(null, resource);
            }
            callback();
        }
    ];

@hardfist thank you so much for the help.

dgmachado commented 1 year ago

Just to confirm can't the newrelic and rest-facade be used inside the lazyImports? is it expected?

hardfist commented 1 year ago

goona check it out

dgmachado commented 1 year ago

@hardfist in addition to your investigation, a have a special issue only in my last app, the one with rest-facade and newrelic bypass, if the bundle is generated with minimize as true I'm getting the below runtime issue:

[Nest] 28424  - 04/27/2023, 6:42:09 AM   ERROR [ew] Nest can't resolve dependencies of the eT (CACHE_MANAGER, ?). Please make sure that the argument Reflector at index [1] is available in the eE context.

Potential solutions:
- Is eE a valid NestJS module?
- If Reflector is a provider, is it part of the current eE?
- If Reflector is exported from a separate @Module, is that module imported within eE?
  @Module({
    imports: [ /* the Module containing Reflector */ ]
  })

Error: Nest can't resolve dependencies of the eT (CACHE_MANAGER, ?). Please make sure that the argument Reflector at index [1] is available in the eE context.

Potential solutions:
- Is eE a valid NestJS module?
- If Reflector is a provider, is it part of the current eE?
- If Reflector is exported from a separate @Module, is that module imported within eE?
  @Module({
    imports: [ /* the Module containing Reflector */ ]
  })

    at eI.lookupComponentInParentModules (/Users/douglasmachado/sample-api/main.js:53:38112)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async eI.resolveComponentInstance (/Users/douglasmachado/sample-api/main.js:53:36917)
    at async ej (/Users/douglasmachado/sample-api/main.js:53:35169)
    at async Promise.all (index 1)
    at async eI.resolveConstructorParams (/Users/douglasmachado/sample-api/main.js:53:35438)
    at async eI.loadInstance (/Users/douglasmachado/sample-api/main.js:53:33866)
    at async eI.loadInjectable (/Users/douglasmachado/sample-api/main.js:53:34398)
    at async Promise.all (index 0)
    at async eP.createInstancesOfInjectables (/Users/douglasmachado/sample-api/main.js:53:46463)

Obs: If the minimize is false the runtime is working well.

hardfist commented 1 year ago

seems a minifier bug,can you file a seperate issue and provide a reproduce demo

dgmachado commented 1 year ago

I've applied this lazyImports into 7 different Nest.js apps and the bypass to newrelic is required and the minify is working well, the only exception is this one with bypass for newrelic and rest-facade that the minify isn't working. I'll try to reproduce a demo as soon as possible.

dgmachado commented 1 year ago

@hardfist Just to be clear, in my scenario, I'm more interested to know if it's expected the newrelic bypass, or if there's another way to fix this (not use the bypass) than the minify issue.

hardfist commented 1 year ago

Just to confirm can't the newrelic and rest-facade be used inside the lazyImports? is it expected?

I think it is expected, bundle node server app is not easy because unlike webapp most of the nodejs third party libraries may not have good support of bundle(like use fs、binary addon and so on), so you just need to handle these libraries case by case or use more robust solution like ncc

dgmachado commented 1 year ago

@hardfist I'm working on this minification issue, How can I disable name mangling?

hardfist commented 1 year ago

@hardfist I'm working on this minification issue, How can I disable name mangling?

optimization.minimize=false

dgmachado commented 1 year ago

@hardfist hacking it was possible validate when I change the @rspack/plugin-minify plugin adding the keep_fnames property as true in the terser plugin as below it fix my runtime issue:

const result = await terser.minify(
                {
                    [sourcefile]: code
                },
                {
                    sourceMap: sourcemap,
                    ecma: this.options.target,
                    keep_fnames: true,
                    compress: {
                        passes: 2
                    }
                }
            );
            return result;

I just created this PR https://github.com/web-infra-dev/rspack/pull/2983 to allow us to customize these properties.

hardfist commented 1 year ago

since we already supports custom minifyOptions of swc minifier, you can use following options to fix it

{
  buitlins: {
     minifyOptions: {
         keep_fnames:false
    }
  }
}
hardfist commented 1 year ago

seems no issue about external dependencies, closed this issue