Skip to content

Conversation

hyj1991
Copy link
Member

@hyj1991 hyj1991 commented Sep 14, 2021

Checklist
  • npm test passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change

support:

{
  "name": "example",
  "eggScriptConfig": {
    "require": [
       "./inject.js"
    ]
  }
}

The current pkgInfo.eggScriptConfig.require need to provide full path, this pr aims to support relative path & npm pkg.

@hyj1991
Copy link
Member Author

hyj1991 commented Sep 14, 2021

Waiting for #48 revert.

@codecov
Copy link

codecov bot commented Sep 14, 2021

Codecov Report

Merging #47 (3b49af1) into master (499dedd) will increase coverage by 0.44%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #47      +/-   ##
==========================================
+ Coverage   99.15%   99.60%   +0.44%     
==========================================
  Files           6        6              
  Lines         237      252      +15     
  Branches       46       50       +4     
==========================================
+ Hits          235      251      +16     
+ Misses          2        1       -1     
Impacted Files Coverage Δ
lib/command.js 100.00% <100.00%> (ø)
lib/helper.js 100.00% <0.00%> (+4.54%) ⬆️

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 499dedd...3b49af1. Read the comment docs.

lib/command.js Outdated
}

// read `egg.require` from package.json
if (argv.require && Array.isArray(argv.require)) {
Copy link
Member

Choose a reason for hiding this comment

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

这段是不是该放在 for 前面,不然 pkgInfo 的 require 和 --require 会覆盖?我们应该是需要合并。

}
}

delete argv.require;
Copy link
Member

@atian25 atian25 Sep 14, 2021

Choose a reason for hiding this comment

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

argv.require 有哪里插入到 execArgvObj.require 了么?(不确定 common-bin 里面的逻辑是否执行了)

下面那个单测,多加个 --require 验证下是否合并了。

Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/eggjs/egg-scripts/pull/47/files/9f3bcf624beb545d40e1b1e6c39251f93e964f7c#diff-d8db36db6bfb32591336006f1dc1adee9e2caf7b021469995aa687c5c5723d0cR61-R64

这边,这样顺便也解决了 --require 只能传递全路径的易用性(并且兼容以前的全路径传递),两边合并下作为 execObject 传递逻辑上清晰一些

Copy link
Member Author

Choose a reason for hiding this comment

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

@atian25 atian25 merged commit 1a7f09c into master Sep 15, 2021
@atian25 atian25 deleted the rollback branch September 15, 2021 06:32
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.

2 participants