Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

logform library uses non-string literals in require and fails at runtime, but works with webpack #480

Closed
alexkli opened this issue Oct 21, 2020 · 7 comments

Comments

@alexkli
Copy link

alexkli commented Oct 21, 2020

The npm library logform does this:

      return require(`./${path}.js`);

The value of path is pretty "static" for a list of local modules.

Running this with esbuild results in this warning:

node_modules/logform/index.js:28:13: warning: This call to "require" will not be bundled because the argument is not a string literal
      return require(`./${path}.js`);
             ~~~~~~~

But at runtime usage of the logform module fails with e.g.

Error: Cannot find module './combine.js'

While I understand that non-string literals are skipped, this actually works fine with webpack (using version 4). So they must have a trick for such a "simple" case that maybe esbuild could replicate.

@evanw
Copy link
Owner

evanw commented Oct 22, 2020

I believe Webpack does this by bundling all files reachable in that directory and all nested directories, which ends up unintentionally including extra stuff in the bundle in this case. I think this is kind of an anti-pattern and I'm not currently planning on including this ability in esbuild. This code in logform can easily be replaced by bare require() calls that are easy for a bundler to analyze.

Webpack has a ton of features both because it's by far the majority bundler and because it's had many years of many contributors adding features. It's not a goal of esbuild to replicate all of Webpack's features, so just because Webpack supports this doesn't necessarily mean esbuild should too, especially if the feature isn't trivial and has negative implications for bundle size.

@alexkli
Copy link
Author

alexkli commented Oct 23, 2020

Makes sense. Unfortunately I don't have influence over logform and the code that is using it. Is there some solution? E.g. some config to manually include all the files from e.g. node_modules/logform/*?

@nettybun
Copy link

nettybun commented Oct 23, 2020

You could hop-over the logform/index.js file entirely by creating your own local version of that index file which points to its dependencies. I've made a repo as an example:

https://github.com/heyheyhello/esbuild-logform-example

https://github.com/heyheyhello/esbuild-logform-example/blob/work/index.js#L1-L16

https://github.com/heyheyhello/esbuild-logform-example/blob/work/logform.js#L1-L22

Basically everywhere in your project that you import "logform" now import "./logform" (or setup a non-relative path in your tsconfig.json/jsconfig.json since esbuild will follow that)

@alexkli
Copy link
Author

alexkli commented Oct 24, 2020

Hmm, that seems a bit involved.

Would the inject option work? IIUC you can provide a list of paths there? Or would the resolution not work in the bundled file?

@evanw
Copy link
Owner

evanw commented Oct 24, 2020

Since esbuild respects paths option in tsconfig.json, you can use a tsconfig.json file to redirect logform to your own file like this:

{
  "compilerOptions": {
    "baseUrl": ".",
    "paths": {
      "logform": [
        "./custom-logform.js"
      ]
    }
  }
}

Where custom-logform.js is logform/index.js modified to avoid dynamic require:

(click to expand custom-logform.js)
'use strict';
 
/*
 * @api public
 * @property {function} format
 * Both the construction method and set of exposed
 * formats.
 */
const format = exports.format = require('logform/format');
 
/*
 * @api public
 * @method {function} levels
 * Registers the specified levels with logform.
 */
exports.levels = require('logform/levels');
 
/*
 * @api private
 * method {function} exposeFormat
 * Exposes a sub-format on the main format object
 * as a lazy-loaded getter.
 */
function exposeFormat(name, requireFormat) {
  Object.defineProperty(format, name, {
    get() {
      return requireFormat();
    },
    configurable: true
  });
}
 
//
// Setup all transports as lazy-loaded getters.
//
exposeFormat('align', function () { return require('logform/align') });
exposeFormat('errors', function () { return require('logform/errors') });
exposeFormat('cli', function () { return require('logform/cli') });
exposeFormat('combine', function () { return require('logform/combine') });
exposeFormat('colorize', function () { return require('logform/colorize') });
exposeFormat('json', function () { return require('logform/json') });
exposeFormat('label', function () { return require('logform/label') });
exposeFormat('logstash', function () { return require('logform/logstash') });
exposeFormat('metadata', function () { return require('logform/metadata') });
exposeFormat('ms', function () { return require('logform/ms') });
exposeFormat('padLevels', function () { return require('logform/pad-levels') });
exposeFormat('prettyPrint', function () { return require('logform/pretty-print') });
exposeFormat('printf', function () { return require('logform/printf') });
exposeFormat('simple', function () { return require('logform/simple') });
exposeFormat('splat', function () { return require('logform/splat') });
exposeFormat('timestamp', function () { return require('logform/timestamp') });
exposeFormat('uncolorize', function () { return require('logform/uncolorize') });

FWIW there is already an open PR against logform that would resolve this issue: winstonjs/logform#113.

@evanw
Copy link
Owner

evanw commented Feb 3, 2021

Closing since there is an esbuild-specific workaround and the authors of that package are aware of the problem and have been provided with a fix.

@evanw evanw closed this as completed Feb 3, 2021
raphaelschwinger added a commit to raphaelschwinger/mongoose that referenced this issue Feb 15, 2021
Importing from paths generated at runtime breaks
parcel, see (parcel-bundler/parcel#4031), or
esbuild, see (evanw/esbuild#480)
By removing the calls to the deprecated global MONGOOSE_DRIVER_PATH this
issue can be resolved.
raphaelschwinger added a commit to raphaelschwinger/mongoose that referenced this issue Feb 15, 2021
Importing from paths generated at runtime breaks
parcel, see (parcel-bundler/parcel#4031), or
esbuild, see (evanw/esbuild#480)
By removing the calls to the deprecated global MONGOOSE_DRIVER_PATH this
issue can be resolved.
@vcfvct
Copy link

vcfvct commented Jun 15, 2021

Hit the same issue, 😞 trying to follow @evanw 's suggestion about using a custom logform.js file, but hitting the below error during tsc, any ideas?
Thanks.

node_modules/winston-transport/index.d.ts:9:26 - error TS7016: Could not find a declaration file for module 'logform'. '/Users/me/GIT/bp/data/custom-logform.js' implicitly has an 'any' type.

9 import * as logform from 'logform';
                           ~~~~~~~~~

node_modules/winston/index.d.ts:8:26 - error TS7016: Could not find a declaration file for module 'logform'. '/Users/me/GIT/bp/data/custom-logform.js' implicitly has an 'any' type.

8 import * as logform from 'logform';

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants