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

fix: when a package is installed using CNPM 'TypeError require(...……) is not a function' occurs #40

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jianxcao
Copy link

When I used CNPM to install the package, 'TypeError require(... ...... ) is not a function error' occurred, mainly because the path of CNPM starts with _ when installing the package, such as 'node_modules/_css-loader@0.28.11@css-loader/lib/css-base.js', so I modified the regular expression to solve this problem

@jhnns
Copy link
Member

jhnns commented Jun 11, 2018

Thanks for the PR 👍

That's really odd. Do you know why cnpm doesn't stick to npm's folder structure?
Could you also explain why cnpm is necessary in the first place? I don't like adding these kind of changes to the code base, but if it solves a problem for a lot of users, it maybe worth it.

@jianxcao
Copy link
Author

Because in China, many places cannot connect to NPM, so CNPM is used instead of NPM, CNPM is the product of alibaba, I don't know why cnpm doesn't stick to npm's folder structure

@imyelo
Copy link

imyelo commented Jun 12, 2018

To simply avoid this problem, use npm (or yarn) with custom registry instead of cnpm might help:

npm config set registry https://registry.npm.taobao.org/

The underscore before the module name is generated by https://github.com/cnpm/npminstall , which works much like yarn, but builds package folders with symbolic link.
For example, if you execute npminstall get-port, you will get these folders in node_modules:

get-port -> [email protected]@get-port
[email protected]@get-port

@jianxcao
Copy link
Author

This approach works, but it is better to be compatible with CNPM, which may be used in the server-side configuration file depending on the location of the server

@@ -40,7 +40,7 @@ function extractLoader(content) {

// If the required file is a css-loader helper, we just require it with node's require.
// If the required file should be processed by a loader we do not touch it (even if it is a .js file).
if (/^[^!]*node_modules[/\\]css-loader[/\\].*\.js$/i.test(absPath)) {
if (/^[^!]*node_modules[/\\](_css-loader@[.\d]+@)*css-loader[/\\].*\.js$/i.test(absPath)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn 't (_css-loader@[.\d]+@)* rather be (_css-loader@[.\d]+@)? ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok,(_css - loader @ [. ] d + @)? It would be better

@jhnns
Copy link
Member

jhnns commented Jun 13, 2018

Ok. Although I don't really see the use-case for cnpm (since you can switch registries), I see the use-case for this fast npm install.

@jianxcao could you also add a test for this? It could be hard to test though, but please give it a try. I don't like to pull in features that are untested because they tend to break later 😁

@jianxcao
Copy link
Author

@jhnns I'll try adding tests

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.

3 participants