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

feat: support customLoader #3484

Merged
merged 12 commits into from
Mar 7, 2019
Merged

feat: support customLoader #3484

merged 12 commits into from
Mar 7, 2019

Conversation

popomore
Copy link
Member

#3480

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

lib/core/custom_loader.js Outdated Show resolved Hide resolved
lib/core/custom_loader.js Outdated Show resolved Hide resolved
test/lib/core/custom_loader.test.js Outdated Show resolved Hide resolved
@popomore popomore changed the title WIP: feat: support customLoader feat: support customLoader Feb 26, 2019
@codecov
Copy link

codecov bot commented Feb 26, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3484      +/-   ##
==========================================
+ Coverage   99.67%   99.67%   +<.01%     
==========================================
  Files          32       32              
  Lines         922      923       +1     
==========================================
+ Hits          919      920       +1     
  Misses          3        3
Impacted Files Coverage Δ
lib/loader/app_worker_loader.js 100% <100%> (ø) ⬆️

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 2f2bd69...a729269. Read the comment docs.

@popomore
Copy link
Member Author

再看看

docs/source/en/advanced/loader.md Outdated Show resolved Hide resolved
// the property name when load to application, E.X. app.controller
controller: {
// relative to app.config.baseDir
directory: 'app/controller',
Copy link
Member

Choose a reason for hiding this comment

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

logger 那个是全路径的,是不是也可以支持下相对路径。

Copy link
Member Author

Choose a reason for hiding this comment

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

这个是相对代码的,而 logger 不知道会在什么目录

Copy link
Member

Choose a reason for hiding this comment

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

我想表达的是,logger 那里应该支持配置相对路径,然后我们判断是相对的,自动 join 上 appInfo.root,这样就不会写死了,也不用写 fn 格式的 config 了,简单很多。

而且修改 root 的话,就可以统一修改 custom logger 的所有文件路径了

Copy link
Member

Choose a reason for hiding this comment

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

原来我之前已经实现了,eggjs/egg-logger#41

lib/core/custom_loader.js Outdated Show resolved Hide resolved
@atian25
Copy link
Member

atian25 commented Feb 27, 2019

放到 egg-core 吧,Midway 好像是基于 egg-core 的,不是 egg

Copy link
Member

@atian25 atian25 left a comment

Choose a reason for hiding this comment

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

那句中文注释改下英文。

+1

@popomore popomore mentioned this pull request Mar 6, 2019
7 tasks

```js
// app.js
// This is only the example below, please use 'loadController' instead
Copy link
Member

Choose a reason for hiding this comment

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

或者把示例改为加载 app/utils,避免误导

Copy link
Member

@atian25 atian25 left a comment

Choose a reason for hiding this comment

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

+1

@popomore popomore merged commit 4cf06da into master Mar 7, 2019
@popomore popomore deleted the custom-loader branch March 7, 2019 07:29
popomore pushed a commit that referenced this pull request Mar 7, 2019
feat: support customLoader (#3484)

#3480
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.

None yet

2 participants