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 by env #158

Merged
merged 3 commits into from
Apr 20, 2018
Merged

feat: support ts by env #158

merged 3 commits into from
Apr 20, 2018

Conversation

whxaxes
Copy link
Member

@whxaxes whxaxes commented Apr 17, 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

支持从 env 中读取 typescript 配置,配合 egg-bin 的 eggjs/bin#98 ,从而能够支持在 egg-schedule 等插件中,直接实例化 FileLoader 的时候,也能加载 ts

@codecov
Copy link

codecov bot commented Apr 17, 2018

Codecov Report

Merging #158 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #158   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          17     17           
  Lines         893    891    -2     
=====================================
- Hits          893    891    -2
Impacted Files Coverage Δ
lib/loader/file_loader.js 100% <100%> (ø) ⬆️
lib/loader/egg_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 fdcde0b...9953ab4. Read the comment docs.

@whxaxes whxaxes requested review from atian25 and popomore April 17, 2018 13:08
@popomore
Copy link
Member

原来的逻辑删了?

@whxaxes
Copy link
Member Author

whxaxes commented Apr 18, 2018

@popomore 原来的不能删,因为之前的版本 egg-bin 不是所有 command 都设置了环境变量,如果原来的删了就 break 了

@atian25
Copy link
Member

atian25 commented Apr 18, 2018

@whxaxes 只要开发者同时升级 egg-bin 和 egg-core 后不 break 就不算吧

删除原来逻辑指的是删除哪些?

@whxaxes
Copy link
Member Author

whxaxes commented Apr 18, 2018

@atian25 是指删除 option,统一由 env 来控制

@popomore
Copy link
Member

应该不会不兼容吧,原来也就兼容了 dev、test、cov 这些命令,现在所有都支持了这个配置了。所以在 egg-core 只要根据这个环境变量来判断就好了。

@popomore
Copy link
Member

你改一下原来的用例,入口都换成这个变量,看是否有问题。

@whxaxes
Copy link
Member Author

whxaxes commented Apr 18, 2018

@popomore 原来 egg-bin 只在 test 命令下才会设置 env

@whxaxes
Copy link
Member Author

whxaxes commented Apr 18, 2018

所以我才提了这个 PR:eggjs/bin#98 给所有 command 加上 env

@whxaxes
Copy link
Member Author

whxaxes commented Apr 18, 2018

或者就先发了 egg-bin 的这个版本,然后我再把 egg-core 改成基于 env ?

@popomore
Copy link
Member

egg-bin 已经发了

@whxaxes
Copy link
Member Author

whxaxes commented Apr 18, 2018

好,我改一下

@atian25
Copy link
Member

atian25 commented Apr 18, 2018

删除 options 的话, egg-scripts, egg-cluster 等地方岂不是也要改?
这种算不算 break?

@whxaxes
Copy link
Member Author

whxaxes commented Apr 18, 2018

@atian25 break 倒不会,只是要改一下 unittest,把 option 全部去掉,ts 开启的链路就会变得比之前简单很多,就是 egg-bin 开启 env,egg-core 读取 env 启用 ts

@whxaxes
Copy link
Member Author

whxaxes commented Apr 18, 2018

已经移除 option 了

@whxaxes
Copy link
Member Author

whxaxes commented Apr 18, 2018

还有其他问题么?

@popomore
Copy link
Member

egg-scripts 应该不会传 typescript 吧

@atian25
Copy link
Member

atian25 commented Apr 18, 2018

@popomore 需要的,要自动加载 sourcemap support 来处理堆栈。

egg-script start --typescript 但可以不传递给 egg-cluster

@popomore
Copy link
Member

好像只是加了个 sourcemap,和 egg-core 没关系

@popomore
Copy link
Member

egg-mock 和 egg-scripts 的再评估一下?这里没啥问题了。

@atian25
Copy link
Member

atian25 commented Apr 19, 2018

https://github.com/eggjs/egg-mock/pull/73/files

这里之前就已经干掉 options 了,再优化下写法应该就 ok 了。

@whxaxes 跟进下。

@atian25
Copy link
Member

atian25 commented Apr 19, 2018

egg-scripts 那个没有传递东西,只是自己 require,应该没事。
https://github.com/eggjs/egg-scripts/blob/master/lib/command.js#L55

@popomore
Copy link
Member

那等 egg-mock 先发?

@atian25
Copy link
Member

atian25 commented Apr 20, 2018

看了下,egg-mock 不需要做任何处理,所以我在 eggjs/mock#73 里面把所有的 ts 相关逻辑删除了。

@popomore
Copy link
Member

那合并了,你来发吧

@popomore
Copy link
Member

发好去 example 测下

@popomore popomore merged commit 9ab4dcf into master Apr 20, 2018
@atian25 atian25 deleted the ts-env branch April 21, 2018 02:52
@atian25
Copy link
Member

atian25 commented Apr 21, 2018

4.7.0

我让 examples 的 travis 重跑了,等会看看结果

@atian25
Copy link
Member

atian25 commented Apr 21, 2018

examples travis 过了。

hyj1991 pushed a commit to hyj1991/egg-core that referenced this pull request Sep 16, 2021
* feat: support ts by env
* feat: check ts in options
* feat: remove typescript option
@@ -415,7 +408,7 @@ class EggLoader {
return undefined;
}

if (!this.options.typescript && fullPath.endsWith('.ts')) {
if (process.env.EGG_TYPESCRIPT !== 'true' && fullPath.endsWith('.ts')) {
Copy link
Member

Choose a reason for hiding this comment

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

为什么只有 EGG_TYPESCRIPT true 才会加载 ts ?

Copy link
Member Author

Choose a reason for hiding this comment

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

因为只有在本地开发才需要加载 ts ,这个环境变量是 egg-bin 控制,如果不通过这个环境变量控制,在部署的时候也会去加载 ts 文件了,但是部署的时候没有 ts-node 就报错了

Copy link
Member

Choose a reason for hiding this comment

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

按道理 require.resolve('/foo/bar') 之后去找 .js 后缀,不会去加载 .ts 后缀的吧。

Copy link
Member Author

Choose a reason for hiding this comment

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

我记得扫文件会扫出带后缀的

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.

4 participants