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

add .json extension for webpack targeting node #173

Closed
wants to merge 1 commit into from

Conversation

OR13
Copy link

@OR13 OR13 commented Sep 29, 2017

Superagent cannot be webpacked targeting node due to the missing .json extension.

ERROR in ./~/mime/index.js
Module not found: Error: Can't resolve './types/standard' in '/Users/user/project/node_modules/mime'
 @ ./~/mime/index.js 4:26-53
 @ ./~/superagent/lib/node/index.js
 @ ./src/lib/api.ts
 @ ./src/index.ts
 @ multi ./src/index.ts

ERROR in ./~/mime/index.js
Module not found: Error: Can't resolve './types/other' in '/Users/user/project/node_modules/mime'
 @ ./~/mime/index.js 4:55-79
 @ ./~/superagent/lib/node/index.js
 @ ./src/lib/api.ts
 @ ./src/index.ts
 @ multi ./src/index.ts

Adding .json, and using a json loader in webpack:

    {
      test: /\.json$/,
      loader: 'json-loader'
    }

resolves this issue.

Superagent needs to be webpacked with target: node outside of web environments.

https://github.com/visionmedia/superagent/wiki/Superagent-for-Webpack

Its possible there is a better way to resolve this in webpack.

@broofa
Copy link
Owner

broofa commented Sep 30, 2017

'Just discussed this in issue #172 (tl;dr - webpack default extensions array includes json. If you override that to not include json then webpack is working as expected.)

Note the bit at the bottom about the CommonJS spec explicitly saying module names should not include extensions.

'Curious to hear your thoughts once you've read through that.

@OR13
Copy link
Author

OR13 commented Oct 2, 2017

@broofa thanks for the reply.

If this is meant to be a CommonJS module it does not make sense to include the extension.

However, if there is no harm in adding it, I don't think CommonJS module system makes any sense.

Being more explicit should be the default.

I've encountered a similar issue with bundling Scrypt which uses such an import, and there is no way to distinguish between the .json, .js and .node files in that package other than the extension.

I agree with what you are saying, and for a module like this that is used by so many projects, its extra important not to break it to resolve a problem with bundlers which can be resolved by the consumer normally.

I've actually repro'd this with both webpack and rollup, here is a relevant ticket for rollup for posterity: rollup/rollup#480

Just another day as a JS dev :)

@OR13 OR13 closed this Oct 2, 2017
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

Successfully merging this pull request may close these issues.

None yet

2 participants