Node-slack-sdk: pkginfo breaks webpack build

[bogdanmarin]

Description

I'm having this issue when importing slack client.

It crashes on the line:

const { WebClient } = require('@slack/client');

I'm using [email protected]

Might be the same as: #445 ?

2018-02-08T17:04:15.485548+00:00 app[web.1]: TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string
2018-02-08T17:04:15.485567+00:00 app[web.1]: at assertPath (path.js:28:11)
2018-02-08T17:04:15.485569+00:00 app[web.1]: at Object.dirname (path.js:1347:5)
2018-02-08T17:04:15.485571+00:00 app[web.1]: at Function.pkginfo.find (/app/node_modules/pkginfo/lib/pkginfo.js:91:16)
2018-02-08T17:04:15.485572+00:00 app[web.1]: at Function.pkginfo.read (/app/node_modules/pkginfo/lib/pkginfo.js:122:22)
2018-02-08T17:04:15.485574+00:00 app[web.1]: at module.exports (/app/node_modules/pkginfo/lib/pkginfo.js:68:21)
2018-02-08T17:04:15.485576+00:00 app[web.1]: at Object. (/app/compiled/server.js:6:104666)
2018-02-08T17:04:15.485578+00:00 app[web.1]: at Object. (/app/compiled/server.js:6:105120)
2018-02-08T17:04:15.485579+00:00 app[web.1]: at t (/app/compiled/server.js:1:155)
2018-02-08T17:04:15.485581+00:00 app[web.1]: at Object. (/app/compiled/server.js:20:3298)
2018-02-08T17:04:15.485583+00:00 app[web.1]: at t (/app/compiled/server.js:1:155)
2018-02-08T17:04:15.485584+00:00 app[web.1]: at Object. (/app/compiled/server.js:20:125024)
2018-02-08T17:04:15.485586+00:00 app[web.1]: at t (/app/compiled/server.js:1:155)
2018-02-08T17:04:15.485590+00:00 app[web.1]: at Object. (/app/compiled/server.js:20:120942)
2018-02-08T17:04:15.485592+00:00 app[web.1]: at t (/app/compiled/server.js:1:155)
2018-02-08T17:04:15.485594+00:00 app[web.1]: at /app/compiled/server.js:20:455159
2018-02-08T17:04:15.485596+00:00 app[web.1]: at model.Query. (/app/node_modules/mongoose/lib/model.js:4074:16)
2018-02-08T17:04:15.485598+00:00 app[web.1]: at /app/node_modules/kareem/index.js:273:21
2018-02-08T17:04:15.485600+00:00 app[web.1]: at /app/node_modules/kareem/index.js:131:16
2018-02-08T17:04:15.485602+00:00 app[web.1]: at process._tickCallback (internal/process/next_tick.js:150:11)

[clavin]

Why the Error is Thrown

This entire issue stems from the use of the pkginfo module by this SDK. The SDK currently uses pkginfo to get some information from this package’s package.json file, namely the version and package name. This information is used as part of the User-Agent header when accessing the Slack API.

In this issue, the SDK is being imported in a browser environment. pkginfo works by using the native path module to locate the parent module’s (the SDK’s) package.json, then reads it using require as an attempt to be bundler-friendly. pkginfo attempts to locate the directory of the SDK by using the filename and id fields of the module object for the SDK (like module.exports).

The problem arises when the bundler used by the user does not provide these fields on the module object. This means that undefined is ultimately passed to path.dirname, which ends up throwing.

---

How to Fix It

As a user of the SDK, there’s really no way to fix this as of right now, other than finding a different bundler or to configuring yours current one differently.

As for the maintainers and contributors for this repository, something can be done: drop the use of pkginfo in favor of a build task that generates a file containing the data used in the User-Agent header. This is a viable solution because the SDK is already planned for TypeScript migration, and since that requires the library to be “built” before publishing, adding an extra build step is very appropriate—though a workaround should be found for testing (implementing an option?).

Alternatively, an options object can be passed as the second argument to pkginfo with the dir option set to the directory of the file using pkginfo or the root directory of the SDK package (either works). I’d advise against this because this can really only be done using some variation of __dirname, which can cause the same problem showcased in this issue.

Edit: as another solution, I don’t see why we can’t just import the package.json directly from its relative path to whatever file needs that info. After all, pkginfo does just that in a more convoluted (with the goal of convenience, mind you) fashion currently.

[aoberoi]

@clavin thanks for digging into this!

in my development branch of v4 with TypeScript, i migrated from using pkginfo to using pjson. that gives me a loose type definition so it resolves one problem i was having, but it works about the same as pkginfo (depends on __dirname), so this issue would still exist.

the two options i see right now are:

1. convince the typescript compiler not to complain on import * as pkg from '../package.json';. the error in my relatively vanilla project configuration is Cannot find module '../package.json'.
2. as @clavin said: write the abbreviated package metadata to another file on build, and then worry about how to import that without the typescript compiler complaining.

these two solutions seem roughly equivalent, so i'm inclined to go with the first.

i do wonder though, is there some conventional behavior of all major bundlers (browserify, webpack, rollup, etc) to "blacklist" a dependency (or transitive dependency) such that the bundler excludes it and replaces it with a stub? i see that browserify uses the package.json field "browser" to accomplish this, but is that respected across bundlers? if so, one very hacky but easy to implement solution is to just wrap a require('../package.json') in a try/catch, and silently fail building the version string. this is used for instrumentation purposes, but would have no functional impact. it still requires this package's maintainers to implement a fix.

[aoberoi]

relevant: https://hackernoon.com/import-json-into-typescript-8d465beded79

[clavin]

I’d opt for simply requiring package.json:

const packageMetadata: IPackageMetadata = require(‘../package.json’);

import syntax should be used when importing ECMAScript modules; JSON files are not ES modules; therefore, JSON files shouldn’t be imported using import syntax.

This also has the added benefit of making it easy for bundler a to identify exactly which package.json file they should be bundling, avoiding bloat and reducing the (likely) possibility that a bundler fails to recognize the package metadata should be included in the bundle.

Not really sure if bundlers still support require, by my intuition is that they do. Nonetheless, I haven’t checked, and thus don’t know.

tl;dr: just require it.

[SebastianEdwards]

Related to this issue: I use serverless-webpack, which creates a small package.json file for each lambda function (containing only the dependencies object). While the new pjson solution in v4 can load these files, it still errors in util.ts which assumes the package.json will contain both name and version keys.

[aoberoi]

i'm going to tag this issue as v3 so that we can separate the pkginfo problem from the pjson problem.

[aoberoi]

i'd like to move forward with the fix that @clavin has suggested (just requiring the package.json directly).

does anyone anticipate problems using this in v3? i've seen some clever, webpack-specific solutions that involve the IgnorePlugin and DefinePlugin, but i think they won't be necessary.

[aoberoi]

The v3 line of this project is no longer supported, so we're closing all issues that were specific to those releases. If this issue still persists in current releases, please open a new issue.