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 ts from env and pkg #71

Merged
merged 2 commits into from
Mar 30, 2018
Merged

feat: support ts from env and pkg #71

merged 2 commits into from
Mar 30, 2018

Conversation

atian25
Copy link
Member

@atian25 atian25 commented Mar 29, 2018

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

@atian25 atian25 requested a review from popomore March 29, 2018 06:07
@@ -39,6 +40,21 @@ module.exports = function formatOptions(options) {
}
options.customEgg = options.framework = framework;

// typescript
if (!options.hasOwnProperty('typescript')) {
if (process.env.EGG_TYPESCRIPT) {
Copy link
Member Author

Choose a reason for hiding this comment

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

两个同时存在的时候,优先级是?

Copy link
Member

Choose a reason for hiding this comment

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

感觉 env 优先更合理一些

Copy link
Member Author

Choose a reason for hiding this comment

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

那就是现在这种了。

Copy link
Member

Choose a reason for hiding this comment

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

所以命令行 > 环境 > pkg

Copy link
Member Author

Choose a reason for hiding this comment

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

这里应该不存在命令行的情况, mm.app 都是手动调用的。
egg-bin 那边想影响这个,只能通过环境变量的方式默认配置

@atian25
Copy link
Member Author

atian25 commented Mar 29, 2018

win 的那个测试还是没搞不明白

@popomore
Copy link
Member

这个 win 是一直挂的?

@atian25
Copy link
Member Author

atian25 commented Mar 29, 2018

嗯,master 前几天也是这里挂的。单独 only 好像没事,一起跑就挂

@codecov
Copy link

codecov bot commented Mar 29, 2018

Codecov Report

Merging #71 into master will decrease coverage by 52.64%.
The diff coverage is 86.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #71       +/-   ##
===========================================
- Coverage   94.89%   42.25%   -52.65%     
===========================================
  Files          12       10        -2     
  Lines         588      568       -20     
===========================================
- Hits          558      240      -318     
- Misses         30      328      +298
Impacted Files Coverage Δ
bootstrap.js 100% <ø> (ø) ⬆️
lib/http_test.js 33.33% <100%> (-66.67%) ⬇️
lib/format_options.js 84.09% <77.77%> (-15.91%) ⬇️
app/middleware/cluster_app_mock.js 9.3% <0%> (-83.73%) ⬇️
app/extend/application.js 18.3% <0%> (-75.82%) ⬇️
lib/cluster.js 19.44% <0%> (-73.15%) ⬇️
index.js 56.52% <0%> (-30.44%) ⬇️
lib/app.js 71.42% <0%> (-27.86%) ⬇️
app.js 80% <0%> (-20%) ⬇️
... and 4 more

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 35d4108...c46a902. Read the comment docs.

@atian25 atian25 force-pushed the ts branch 7 times, most recently from c46a902 to 50ee2d2 Compare March 30, 2018 03:26
@atian25
Copy link
Member Author

atian25 commented Mar 30, 2018

@popomore#65 就已经挂了。跟了下 supertest 源码,在 end 里面的 res 不知道为啥为空了,回头再看吧

@@ -39,6 +40,21 @@ module.exports = function formatOptions(options) {
}
options.customEgg = options.framework = framework;

// typescript
if (!options.hasOwnProperty('typescript')) {
Copy link
Member

Choose a reason for hiding this comment

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

不要 hasOwnProperty 吧

Copy link
Member Author

@atian25 atian25 Mar 30, 2018

Choose a reason for hiding this comment

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

options. typescript === undefined ?

Copy link
Member

@popomore popomore Mar 30, 2018

Choose a reason for hiding this comment

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

'typescript; in options?

测试是不是就提供环境变量就好了

const baseDir = path.join(__dirname, 'fixtures/demo');
mm(process, 'cwd', () => baseDir);
mm(process.env, 'EGG_TYPESCRIPT', true);
const opts = formatOptions();
Copy link
Member

Choose a reason for hiding this comment

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

这个不是应该传 typescript 测一下优先级?

@@ -39,6 +40,21 @@ module.exports = function formatOptions(options) {
}
options.customEgg = options.framework = framework;

// typescript
if (!options.hasOwnProperty('typescript')) {
if (process.env.EGG_TYPESCRIPT) {
Copy link
Member

Choose a reason for hiding this comment

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

所以命令行 > 环境 > pkg

const pkgFile = path.join(options.baseDir, 'package.json');
if (fs.existsSync(pkgFile)) {
const pkgInfo = require(pkgFile);
if (pkgInfo && pkgInfo.egg && pkgInfo.egg.typescript) {
Copy link
Member

Choose a reason for hiding this comment

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

pkgInfo.egg.typescript === true

// typescript
if (!('typescript' in options)) {
// process.env maybe force to string type
if (String(process.env.EGG_TYPESCRIPT).toLowerCase() === 'true') {
Copy link
Member Author

Choose a reason for hiding this comment

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

这里 env 配置的时候,都会转换为 string 的,所以要这么处理

@atian25
Copy link
Member Author

atian25 commented Mar 30, 2018

@popomore 可以再看看

@popomore
Copy link
Member

可以合并了,覆盖率没更新

@atian25 atian25 merged commit b27c60a into master Mar 30, 2018
@atian25 atian25 deleted the ts branch March 30, 2018 09:23
@atian25
Copy link
Member Author

atian25 commented Mar 30, 2018

3.17.0

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.

3 participants