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

Styleguidist not working with npm 2 (error in make-webpack-config.js) #131

Closed
mik01aj opened this issue Apr 21, 2016 · 7 comments
Closed

Comments

@mik01aj
Copy link
Collaborator

mik01aj commented Apr 21, 2016

The problem is here: https://github.com/sapegin/react-styleguidist/blob/3a7271fc59ada1bcb959d62d174d64fecdc69645/src/make-webpack-config.js#L73

getPackagePath('entities') breaks, because entities is not a dependency of styleguidist. entities is a dependency of markdown-it, which is a dependency of styleguidist, so:

  • when installed with npm 2, the entities module is not accessible from styleguidist,
  • when installed with npm 3, the entities module is not guaranteed to be always accessible from styleguidist (because if some of other dependencies starts depending on another version of entities package, npm 3 will nest them).

@sapegin, could you explain why is the json loader needed there?

Maybe we could change the regex to target specifically these files? Or the other way round, enable it for any *.json files?

@sapegin
Copy link
Member

sapegin commented Apr 21, 2016

  1. It’s needed because entities module uses some JSON files and it’s required by Markdown It.
  2. We shouldn’t enable any loaders that could affect user’s code.

But do we actually need to support npm 2?

As a quick fix we could try to replace getPackagePath('entities') with:

[
  getPackagePath('entities'),
  getPackagePath('markdown-it')
]

@mik01aj
Copy link
Collaborator Author

mik01aj commented Apr 21, 2016

But do we actually need to support npm 2?

Yes, because of Node 4.x LTS releases. They are pretty new and they still use npm 2.

And your quick fix will not work, because getPackagePath('entities') fails with an exception.

I'm actually surprised that Webpack doesn't support json by default. On the other hand, it would be nice if entities wouldn't require json files...

For the time being, I propose to just change these lines like so:

            {
                // TODO: remove this then entities module is fixed (https://github.com/fb55/entities/pull/26)
                test: /node_modules\/entities\/.*\.json$/,
                loader: 'json'
            },

I wouldn't use include because it's hard to predict where this module will land.

@sapegin
Copy link
Member

sapegin commented Apr 21, 2016

Looks good for me.

@mik01aj
Copy link
Collaborator Author

mik01aj commented Apr 21, 2016

We shouldn’t enable any loaders that could affect user’s code.

On the other hand, we already force users to use ES6+JSX in examples, with Lodash 4 and React 15. Anyone using other versions of these libraries, or other Babel config will have problems with writing practical examples. I think that this is more important then avoiding enabling the json loader for user files. ;)

As for the fix - I'll make a PR. But I've seen this:

function validateWebpackConfig(webpackConfig) {
    webpackConfig.module.loaders.forEach(function(loader) {
        if (!loader.include && !loader.exclude) {
            throw Error('Styleguidist: "include" option is missed for ' + loader.test + ' Webpack loader.');
        }
    });
}

Shall I add include: node_modules to the loader then?

@sapegin
Copy link
Member

sapegin commented Apr 21, 2016

I think that this is more important

But there’s no easy fix if it’s possible to fix at all.

Shall I add include: node_modules to the loader then?

Yeah, I think so.

mik01aj pushed a commit to lovelybooks/react-styleguidist that referenced this issue Apr 21, 2016
sapegin added a commit that referenced this issue Apr 21, 2016
@sapegin sapegin closed this as completed Apr 21, 2016
@stepancar
Copy link
Member

@mik01aj, error when i use 2.2.1 and npm 3.8.6
screenshot_1

@mik01aj
Copy link
Collaborator Author

mik01aj commented Apr 29, 2016

Yes, this was already reported in #135.

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

3 participants