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: cnpm install error without user settings #301

Merged
merged 7 commits into from
May 8, 2019
Merged

Conversation

Moudicat
Copy link
Contributor

@Moudicat Moudicat commented May 7, 2019

根据 PR #259 来看,如果用户新装cnpm、指定过registry、且未登录的情况,会导致安装报错。

✖ Install fail! TypeError [ERR_INVALID_ARG_TYPE]: The first argument must be one of type string, Buffer, ArrayBuffer, Array, or Array-like Object. Received type undefined

现先判断是否有用户登录数据再做处理。

@fengmk2
Copy link
Member

fengmk2 commented May 7, 2019

call @hyj1991

@Moudicat 能否增加测试用例?

@codecov
Copy link

codecov bot commented May 7, 2019

Codecov Report

Merging #301 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #301      +/-   ##
==========================================
+ Coverage   92.99%   92.99%   +<.01%     
==========================================
  Files          29       29              
  Lines        2026     2027       +1     
  Branches      349      350       +1     
==========================================
+ Hits         1884     1885       +1     
  Misses        142      142
Impacted Files Coverage Δ
lib/get.js 98.3% <100%> (+0.02%) ⬆️
lib/utils.js 90.72% <0%> (-0.34%) ⬇️
lib/local_install.js 98.23% <0%> (+0.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef1432a...26abbae. Read the comment docs.

@codecov
Copy link

codecov bot commented May 7, 2019

Codecov Report

Merging #301 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #301      +/-   ##
==========================================
+ Coverage   92.99%   92.99%   +<.01%     
==========================================
  Files          29       29              
  Lines        2026     2027       +1     
  Branches      349      350       +1     
==========================================
+ Hits         1884     1885       +1     
  Misses        142      142
Impacted Files Coverage Δ
lib/get.js 98.3% <100%> (+0.02%) ⬆️
lib/utils.js 90.72% <0%> (-0.34%) ⬇️
lib/local_install.js 98.23% <0%> (+0.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef1432a...71dccc7. Read the comment docs.

@Moudicat
Copy link
Contributor Author

Moudicat commented May 7, 2019

大概是这个意思,对写测试用例不是很熟悉,希望指正。

@hyj1991
Copy link
Contributor

hyj1991 commented May 8, 2019

大概是这个意思,对写测试用例不是很熟悉,希望指正。

我试了下 cnpm 指定这个带上 auth 的 npminstall,安装模块并不会有错误,你的错误复现条件可以描述下么?

@Moudicat
Copy link
Contributor Author

Moudicat commented May 8, 2019

大概是这个意思,对写测试用例不是很熟悉,希望指正。

我试了下 cnpm 指定这个带上 auth 的 npminstall,安装模块并不会有错误,你的错误复现条件可以描述下么?

嗯嗯,刚才确认了是部分情况下才会有问题。

复现步骤:

  1. 找一台没有安装过cnpm的机器 or 执行以下步骤
rm ~/.cnpmrc
sudo npm uninstall -g cnpm
  1. 安装cnpm
sudo npm install -g cnpm
  1. 切换cnpm源至指定私有源
cnpm config set registry http://10.10.100.100:7001
  1. 任意安装一个包
cnpm i http-server
  1. 安装失败
✖ Install fail! TypeError [ERR_INVALID_ARG_TYPE]: The first argument must be one of type string, Buffer, ArrayBuffer, Array, or Array-like Object. Received type undefined
TypeError [ERR_INVALID_ARG_TYPE]: The first argument must be one of type string, Buffer, ArrayBuffer, Array, or Array-like Object. Received type undefined
    at Function.from (buffer.js:204:11)
    at get (/usr/local/lib/node_modules/cnpm/node_modules/npminstall/lib/get.js:51:79)
    at get.next (<anonymous>)
    at onFulfilled (/usr/local/lib/node_modules/cnpm/node_modules/co/index.js:65:19)
    at /usr/local/lib/node_modules/cnpm/node_modules/co/index.js:54:5
    at new Promise (<anonymous>)
    at co (/usr/local/lib/node_modules/cnpm/node_modules/co/index.js:50:10)
    at toPromise (/usr/local/lib/node_modules/cnpm/node_modules/co/index.js:118:63)
    at next (/usr/local/lib/node_modules/cnpm/node_modules/co/index.js:99:29)
    at onFulfilled (/usr/local/lib/node_modules/cnpm/node_modules/co/index.js:69:7)

@hyj1991
Copy link
Contributor

hyj1991 commented May 8, 2019

@Moudicat OK,明白了,相当于设置了 registry 但是没有登录,会被判断为需要携带 Auth 信息导致出错

lib/get.js Outdated Show resolved Hide resolved
@hyj1991
Copy link
Contributor

hyj1991 commented May 8, 2019

cnpm 也打算带上鉴权信息的话,后面可以考虑移植 npm-cli 那边的对应逻辑 @fengmk2 .
上次的 Pr 实际上主要是给阿里云的模块仓库使用的,因为那边是强制登陆后才能用,而且私有包的鉴权逻辑是控制在我们自己这边,因此这里的鉴权逻辑写的比较简单。

@fengmk2
Copy link
Member

fengmk2 commented May 8, 2019

@hyj1991 可以合并?

@fengmk2
Copy link
Member

fengmk2 commented May 8, 2019

@Moudicat 测试用例使用 coffee 来跑,新起进程的方式,就不需要这样 hack require cache 实现了。

Copy link
Contributor

@hyj1991 hyj1991 left a comment

Choose a reason for hiding this comment

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

+1

@Moudicat
Copy link
Contributor Author

Moudicat commented May 8, 2019

@Moudicat 测试用例使用 coffee 来跑,新起进程的方式,就不需要这样 hack require cache 实现了。

好的

yield coffee.fork(npminstall, ['webpack-parallel-uglify-plugin@1.0.0'], {
cwd: cwd,
env: Object.assign({}, process.env, {
NODE: cwd,
Copy link
Member

Choose a reason for hiding this comment

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

这个不需要设置吧

Copy link
Contributor Author

Choose a reason for hiding this comment

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

是的,现在保留 USERPROFILEHOME

@fengmk2 fengmk2 added the bug label May 8, 2019
@fengmk2 fengmk2 merged commit 2688fa8 into cnpm:master May 8, 2019
@fengmk2
Copy link
Member

fengmk2 commented May 8, 2019

3.21.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants