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: should sync missing public scoped package on install #946

Merged
merged 3 commits into from Jun 5, 2016

Conversation

Projects
None yet
5 participants
@fengmk2
Member

fengmk2 commented Jun 5, 2016

closes #938

@fengmk2 fengmk2 added the bug label Jun 5, 2016

@fengmk2 fengmk2 added this to the 2.x milestone Jun 5, 2016

@mention-bot

This comment has been minimized.

mention-bot commented Jun 5, 2016

By analyzing the blame information on this pull request, we identified @dead-horse to be a potential reviewer

@codecov-io

This comment has been minimized.

codecov-io commented Jun 5, 2016

Current coverage is 88.54%

Merging #946 into master will increase coverage by <.01%

  1. File ...ync_module_worker.js (not in diff) was modified. more
    • Misses +2
    • Partials 0
    • Hits -2
  2. File ...stry/package/list.js was modified. more
    • Misses -3
    • Partials 0
    • Hits +3
@@             master       #946   diff @@
==========================================
  Files            85         85          
  Lines          3289       3291     +2   
  Methods         340        340          
  Messages          0          0          
  Branches        612        613     +1   
==========================================
+ Hits           2911       2914     +3   
+ Misses          378        377     -1   
  Partials          0          0          

Sunburst

Powered by Codecov. Last updated by 9b572d7...f0170d2

return yield* next;
var scope = name.split('/')[0];
// if not support scopes, don't sync
if (config.scopes.length === 0 || config.scopes.indexOf(scope) >= 0) {

This comment has been minimized.

@dead-horse

dead-horse Jun 5, 2016

Member

不支持 scope 的时候为什么不同步?不支持 scope 也还是要同步公共的带 scope 的模块吧?

This comment has been minimized.

@fengmk2

fengmk2 Jun 5, 2016

Member

那得改一下原来的测试用例了,我改改。

mm(config, 'scopes', []);
request(app)
.get('/@invalid/test')
.expect(404, done);
.expect('Location', 'https://registry.npmjs.com/@invalid/test')
.expect(302, done);

This comment has been minimized.

@fengmk2

@fengmk2 fengmk2 force-pushed the install-sync-on-public-scoped-package branch from 95691dd to 0b0bf0c Jun 5, 2016

if (name && name[0] === '@') {
return yield* next;
var scope = name.split('/')[0];
if (config.scopes.indexOf(scope) >= 0) {

This comment has been minimized.

@dead-horse

This comment has been minimized.

@fengmk2

fengmk2 Jun 5, 2016

Member

好多代码都是直接依赖了 config.scopes 的,都没有做 null 检查,不管了吧。

image

@dead-horse dead-horse merged commit 94dc130 into master Jun 5, 2016

3 checks passed

codecov/project 88.57% (+<.01%) compared to 9b572d7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@dead-horse

This comment has been minimized.

Member

dead-horse commented Jun 5, 2016

+1

@dead-horse dead-horse deleted the install-sync-on-public-scoped-package branch Jun 5, 2016

@dead-horse

This comment has been minimized.

Member

dead-horse commented Jun 5, 2016

2.10.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment