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 plugin.{env}.js #20

Merged
merged 4 commits into from Oct 19, 2016
Merged

feat: support plugin.{env}.js #20

merged 4 commits into from Oct 19, 2016

Conversation

popomore
Copy link
Member

@popomore popomore commented Oct 17, 2016

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

load plugin

Description of change

Control bundle of plugins by env easily

@mention-bot
Copy link

@popomore, thanks for your PR! By analyzing the history of the files in this pull request, we identified @gxcsoccer to be a potential reviewer.

@atian25
Copy link
Member

atian25 commented Oct 17, 2016

plugin 也要支持 env?

// note: can't use for-of
for (let i = 0, l = configPaths.length; i < l; i++) {
const configPath = configPaths[i];
configPaths.push(configPath.replace(/plugin.js$/, `plugin.${this.serverEnv}.js`));
Copy link
Member

Choose a reason for hiding this comment

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

为什么要改 configPaths 本身?

Copy link
Member

Choose a reason for hiding this comment

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

/plugin.js$/ => /\/plugin.js$/ 少了一个 /

@popomore
Copy link
Member Author

这样不用 concat 了

@popomore
Copy link
Member Author

popomore commented Oct 17, 2016

@atian25 这样可以根据环境变量控制插件集合

@atian25
Copy link
Member

atian25 commented Oct 17, 2016

那是不是之前那个开发环境的插件可以设置为 devdep需求就实现了?

@atian25
Copy link
Member

atian25 commented Oct 17, 2016

config 是没有 config.js 的,那 plugin.js 也要改为 plugin.default.js 么?

后面这句当我没说 还是说类似 middleware 直接合并到 config?不用单独文件?

Control bundle of plugins by env easily
@popomore
Copy link
Member Author

这里的 env 大部分是自定义的,感觉不需要类似 config 那样的。这个和 eggPlugin.env 有点冲突,感觉由应用来控制更合理,插件本身控制用在哪种环境感觉太分散了。

@codecov-io
Copy link

codecov-io commented Oct 19, 2016

Current coverage is 99.83% (diff: 100%)

Merging #20 into master will decrease coverage by <.01%

@@             master        #20   diff @@
==========================================
  Files            16         16          
  Lines           619        615     -4   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits            618        614     -4   
  Misses            1          1          
  Partials          0          0          

Powered by Codecov. Last update 404486d...321151e

@atian25
Copy link
Member

atian25 commented Oct 19, 2016

serverEnv 不是只有我们预设的值么? 还可以为其他的?

@popomore
Copy link
Member Author

对,希望支持自定义的。

@popomore
Copy link
Member Author

如果支持自定义,那么插件配置 env 就会很奇怪了。

@atian25
Copy link
Member

atian25 commented Oct 19, 2016

那光改这里还不够吧, 如果自定义的话, 有可能还需要 egg-cluster 里面的 isProduction 的判断.

我上面那个疑问的意思是: 如果当前是 prod, 而某个插件是在 plugin.local.js 中定义的, 那它是否还会被 require?

@popomore
Copy link
Member Author

肯定不会啊

@atian25
Copy link
Member

atian25 commented Oct 19, 2016

那就变相实现之前的那个需求了.

另外:

  • 我上面提到的 config 没有 config.js 而 plugin 会有 plugin.default.js 和 plugin.js, 开发者会疑惑吧.
  • 配置插件 env 的就有三个地方了, 要看看
    • plugin.{env}.js
    • plugin.js 中的 env
    • 插件本身 package.json 的 eggPlugin.env

@popomore
Copy link
Member Author

plugin.js 中的 env 也是向下兼容的,如果有 plugin.env.js 感觉这种写法根本没必要了。问题回到“谁来控制插件开关”。

@popomore
Copy link
Member Author

现在生产环境可能存在同一套代码多套部署的情况,如果支持这种方式,那么可以定义多个环境变量,每个环境变量定义差异的插件集合,部署时通过环境来切换。

@fengmk2
Copy link
Member

fengmk2 commented Oct 19, 2016

+1

@fengmk2
Copy link
Member

fengmk2 commented Oct 19, 2016

merge from master, wait for ci pass.

@fengmk2 fengmk2 merged commit e254fff into master Oct 19, 2016
@fengmk2 fengmk2 deleted the load-plugin-by-env branch October 19, 2016 16:46
fengmk2 added a commit that referenced this pull request Jan 13, 2023
```bash
egg start timeline:
▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ [289ms] - #0 Process Start
                  ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ [510ms] - #1 Application Start
                  ▇ [2ms] - #2 Load Plugin
                  ▇ [1ms] - #3 Load Config
                  ▇ [0ms] - #4 Require(0) ~/eggjs/egg-core/test/fixtures/egg/config/config.default.js
                  ▇ [0ms] - #5 Require(1) ~/eggjs/egg-core/test/fixtures/egg/config/config.unittest.js
                  ▇ [0ms] - #6 Load extend/application.js
                  ▇ [0ms] - #7 Require(2) ~/eggjs/egg-core/test/fixtures/egg/app/extend/application.js
                  ▇ [1ms] - #8 Load extend/context.js
                  ▇ [0ms] - #9 Load extend/request.js
                  ▇ [0ms] - #10 Load extend/response.js
                  ▇ [1ms] - #11 Load app.js
                  ▇ [0ms] - #12 Require(3) ~/eggjs/egg-core/test/fixtures/egg/node_modules/session/app.js
                  ▇ [0ms] - #13 Require(4) app.js
                  ▇▇▇▇▇▇ [101ms] - #14 readyCallback in a
                  ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ [501ms] - #15 readyCallback in b
                  ▇ [4ms] - #16 Load Middleware
                  ▇ [4ms] - #17 Load "middlewares" to Application
                  ▇ [2ms] - #18 Load Service
                  ▇ [2ms] - #19 Load "service" to Context
                  ▇ [0ms] - #20 Load Controller
                  ▇ [0ms] - #21 Load "controller" to Application
                  ▇ [0ms] - #22 Load Router
```
fengmk2 added a commit that referenced this pull request Jan 13, 2023
```bash
egg start timeline:
▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ [289ms] - #0 Process Start
                  ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ [510ms] - #1 Application Start
                  ▇ [2ms] - #2 Load Plugin
                  ▇ [1ms] - #3 Load Config
                  ▇ [0ms] - #4 Require(0) ~/eggjs/egg-core/test/fixtures/egg/config/config.default.js
                  ▇ [0ms] - #5 Require(1) ~/eggjs/egg-core/test/fixtures/egg/config/config.unittest.js
                  ▇ [0ms] - #6 Load extend/application.js
                  ▇ [0ms] - #7 Require(2) ~/eggjs/egg-core/test/fixtures/egg/app/extend/application.js
                  ▇ [1ms] - #8 Load extend/context.js
                  ▇ [0ms] - #9 Load extend/request.js
                  ▇ [0ms] - #10 Load extend/response.js
                  ▇ [1ms] - #11 Load app.js
                  ▇ [0ms] - #12 Require(3) ~/eggjs/egg-core/test/fixtures/egg/node_modules/session/app.js
                  ▇ [0ms] - #13 Require(4) app.js
                  ▇▇▇▇▇▇ [101ms] - #14 readyCallback in a
                  ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ [501ms] - #15 readyCallback in b
                  ▇ [4ms] - #16 Load Middleware
                  ▇ [4ms] - #17 Load "middlewares" to Application
                  ▇ [2ms] - #18 Load Service
                  ▇ [2ms] - #19 Load "service" to Context
                  ▇ [0ms] - #20 Load Controller
                  ▇ [0ms] - #21 Load "controller" to Application
                  ▇ [0ms] - #22 Load Router
```
atian25 pushed a commit that referenced this pull request Jan 13, 2023
[skip ci]

## [5.3.0](v5.2.0...v5.3.0) (2023-01-13)

### Features

* support show app.timing in timline string ([#260](#260)) ([5b7af12](5b7af12)), closes [#0](https://github.com/eggjs/egg-core/issues/0) [#1](#1) [#2](#2) [#3](#3) [#4](#4) [#5](#5) [#6](#6) [#7](#7) [#8](#8) [#9](#9) [#10](#10) [#11](#11) [#12](#12) [#13](#13) [#14](#14) [#15](#15) [#16](#16) [#17](#17) [#18](#18) [#19](#19) [#20](#20) [#21](#21) [#22](#22)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants