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 to require package. Fixes webpack v2 beeing unable… #271

Closed
wants to merge 1 commit into from

Conversation

cgrossde
Copy link

@cgrossde cgrossde commented Feb 3, 2017

Webpack v2 doesn't allow empty extensions anymore: webpack/webpack#3043

Running webpack v2 on a project with got will result in the following error:

ERROR in ./~/got/index.js
Module not found: Error: Can't resolve './package' in '...'
 @ ./~/got/index.js 19:12-32
 @ ./~/is-reachable/index.js
...

Adding the .json extension to require('./package') => require('./package.json') fixes the problem.

@sindresorhus
Copy link
Owner

I don't really see how that's something Got should fix. It is Webpack not following the Node.js resolution logic.

// @TheLarkInn

@julien-f
Copy link
Contributor

julien-f commented Feb 3, 2017

@sindresorhus I think it makes sense to add extensions for non-js files.

Anyway it seems like a fairly minor change to help people using Webpack.

@cgrossde
Copy link
Author

cgrossde commented Feb 3, 2017

@sindresorhus I agree that this is webpack not following require convention. However you would do me a big favour in merging this. It would be an easy fix for anyone using webpack with your package. It could be weeks until webpack fixes this. It also has no effect on your code.

@TheLarkInn
Copy link

@sindresorhus what part of the pattern is off?

@TheLarkInn
Copy link

Um so webpack does have extentions for .js and .json by default (in that respective order)

@TheLarkInn
Copy link

@cgrossde can you please share your webpack config for me, I have a suspicion that you have resolveExtentions overrides incorrectly

@cgrossde
Copy link
Author

cgrossde commented Feb 5, 2017

@TheLarkInn, damn you are right. I was migrating vom webpack v1 to v2 and never thought of adding new extensions, just removed the empty one.
Before (webpack v1):

resolve: {
    extensions: ['', '.js', '.jsx', '.scss']
  },

After (webpack v2) - results in the mentioned error:

resolve: {
    extensions: ['.js', '.jsx', '.scss']
  },

Solution:

resolve: {
    extensions: ['.js', '.jsx', '.scss', '.json]
  },

Sry guys, was working on it after a long day at work. Missed the obvious.

@cgrossde cgrossde closed this Feb 5, 2017
@TheLarkInn
Copy link

@sindresorhus thanks fur the ping 🐶

@TheLarkInn
Copy link

@cgrossde no prob you can always check webpack.js.org/configuration to see the defaults. In this case the array is an complete override so order is important.

@TayBill
Copy link

TayBill commented Apr 24, 2017

Ran into this problem with an Angular2 based app, the angularcli, and default assumptions about webpack. The recommended approach was was run ng eject to generate a non default webpack config file. That all worked, but ended up making big changes to my package.json. The changes were big enough that I'm not going to continue with this approach. Bottom line: It sure would be nice if this merge request (changing from package to package.json) was accepted. It would make things a lot simpler. See also #285

@sindresorhus
Copy link
Owner

@TheLarkInn Webpack should resolve .json in sub-dependencies no matter what people define in the extensions option. This is how the Node.js require resolving works and by giving people ability to shoot themselves in the foot by forgetting to include .json when they override the option, Webpack ends up just wasting everyone's time in issues like this. (I've had to deal with this problem at least 10 times in the past).

I'm not going to change anything in Got as it's Webpack that is not compatible with Node.js and I'm tired of all the churn. People always want the easy solution instead of fixing the actual problem.

Repository owner locked and limited conversation to collaborators Apr 24, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants