launchdarkly / node-server-sdk

LaunchDarkly Server-side SDK for Node
Other
79 stars 65 forks source link

Module not found: Error: Can't resolve 'yaml' #228

Closed maufarinelli closed 3 years ago

maufarinelli commented 3 years ago

Is this a support request? This issue tracker is maintained by LaunchDarkly SDK developers and is intended for feedback on the SDK code. If you're not sure whether the problem you are having is specifically related to the SDK, or to the LaunchDarkly service overall, it may be more appropriate to contact the LaunchDarkly support team; they can help to investigate the problem and will consult the SDK team if necessary. You can submit a support request by going here or by emailing support@launchdarkly.com.

Note that issues filed on this issue tracker are publicly accessible. Do not provide any private account information on your issues. If your problem is specific to your account, you should submit a support request as described above.

Describe the bug While ts compiling our code right now, we have the following error

WARNING in ./node_modules/launchdarkly-node-server-sdk/file_data_source.js 17:19-34
Module not found: Error: Can't resolve 'yaml' in '../src/services/node/node_modules/launchdarkly-node-server-sdk'
 @ ./node_modules/launchdarkly-node-server-sdk/index.js 4:23-52
 @ ./common/utils/launchdarkly.ts 43:53-92

when I see your package.json, it is a devDependency. Should it be a dependency? I see yaml is required inside ./node_modules/launchdarkly-node-server-sdk/file_data_source.js

function FileDataSource(options) {
  if (yamlAvailable === undefined) {
    try {
      const yaml = require('yaml');
      yamlAvailable = true;
      yamlParser = yaml.parse;
    } catch (err) {
      yamlAvailable = false;
    }
  }

To reproduce import LaunchDarkly from 'launchdarkly-node-server-sdk'; and compile your code to typescript

Expected behavior It should compile without any error.

Logs If applicable, add any log output related to your problem.

SDK version The version of this SDK that you are using. Using Version : "launchdarkly-node-server-sdk": "6.2.0",

Language version, developer tools For instance, Go 1.11 or Ruby 2.5.3. If you are using a language that requires a separate compiler, such as C, please include the name and version of the compiler too. Typescript v4.2.4

OS/platform For instance, Ubuntu 16.04, Windows 10, or Android 4.0.3. If your code is running in a browser, please also include the browser type and version.

Additional context Add any other context about the problem here.

eli-darkly commented 3 years ago

yaml is deliberately in devDependencies rather than dependencies because it is optional— the SDK does not need it and we don't want it to be included in application bundles by default. The SDK detects its presence and uses it if it is present (as described in https://launchdarkly.github.io/node-server-sdk/index.html#filedatasource).

It looks like TypeScript does not like this way of doing things. Possibly what we should be doing to get the same behavior is to put yaml in peerDependencies, and then mark it as optional like this:

"peerDependenciesMeta": {
    "yaml": { "optional": true }
  }
eli-darkly commented 3 years ago

@maufarinelli - I'm having trouble reproducing this. Your description under "To reproduce" is pretty general, and I'm guessing that there is some difference between your project and the TypeScript project I created that affects this behavior - either the configuration of TypeScript itself (target JS version, module handling, etc.) or something about how imports are used in your code. Could you provide a minimal project that demonstrates the problem? I think it is likely a real issue, but if I can't reproduce it, it's going to be hard to verify a fix.

maufarinelli commented 3 years ago

Sure @eli-darkly Our tsconfig.json look like this:

{
    "compilerOptions": {
        "esModuleInterop": true,
        "strictNullChecks": true,
        "allowJs": true,
        "target": "es5",
        "module": "commonjs",
        "noImplicitAny": true,
        "preserveConstEnums": true,
        "outDir": "./built",
        "sourceMap": true,
        "experimentalDecorators": true,
        "emitDecoratorMetadata": true,
        "resolveJsonModule": true,
        "downlevelIteration": true,
        "lib": [
            "es5",
            "es2015.promise"
        ],
    },
    "include": [
        "./**/*"
    ],
    "exclude": [
        "node_modules",
        "built.zip",
        "built"
    ]
}

We are importing for now as: import LaunchDarkly from 'launchdarkly-node-server-sdk';

I tried also to import only what we need, but the error persists import { init, LDClient, LDUser, LDFlagValue } from 'launchdarkly-node-server-sdk';

eli-darkly commented 3 years ago

Thanks. Well... I still can't reproduce it, unfortunately.

Just to help narrow down whether this is a difference in our environments or in the project, would you mind trying to build the attached project and letting me know if it reproduces the error for you?

Also, does your compile step consist of anything other than tsc? I'm having trouble seeing how a regular TypeScript compilation run could possibly produce the kind of error you're seeing here— it should not have any reason to be trying to analyze dependencies of already-installed JS modules at that point. The SDK is just JS code, TypeScript should not need to be transforming it in any way, and there is no runtime reason why you can't do a require of an optional module like that; I would've expected it to only be an issue in the context of some kind of bundling step.

(Also, what is your Node version? Not necessarily related, but it is one of the standard questions we ask in the issue template; the TypeScript version alone doesn't answer that.) node-ts-test.zip

maufarinelli commented 3 years ago

I am sorry @eli-darkly , you are totally right! As you said The SDK is just JS code, TypeScript should not need to be transforming it in any way.

Our mistake was that we thought we were excluding node_modules when webpack was compiling our Typescript just doing this: webpack.config.json

...
module: {
    rules: [
      {
        test: /\.tsx?$/,
        use: 'ts-loader',
        exclude: /node_modules/,
      },
    ],
  },
  ...

But actually we needed also to put webpack-node-externals:

const nodeExternals = require('webpack-node-externals');
... 
module: {
    rules: [
      {
        test: /\.tsx?$/,
        use: 'ts-loader',
        exclude: /node_modules/,
      },
    ],
  },
  externals: [nodeExternals()],
  ...

(Classic Mistake)

Sorry again and thanks for all your work!

eli-darkly commented 3 years ago

Oh good! Glad it worked out.