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: auto register tsconfig-paths on env.EGG_TYPESCRIPT enable #254

Merged
merged 1 commit into from
Dec 19, 2022

Conversation

fengmk2
Copy link
Member

@fengmk2 fengmk2 commented Dec 19, 2022

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

@codecov
Copy link

codecov bot commented Dec 19, 2022

Codecov Report

Base: 99.75% // Head: 99.75% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (41725d7) compared to base (877b4f3).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #254   +/-   ##
=======================================
  Coverage   99.75%   99.75%           
=======================================
  Files          19       19           
  Lines        1207     1209    +2     
  Branches      211      212    +1     
=======================================
+ Hits         1204     1206    +2     
  Misses          3        3           
Impacted Files Coverage Δ
lib/loader/egg_loader.js 100.00% <ø> (ø)
lib/egg.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@fengmk2 fengmk2 merged commit 6d75322 into master Dec 19, 2022
@fengmk2 fengmk2 deleted the auto-require-tsconfig-paths branch December 19, 2022 05:18
atian25 pushed a commit that referenced this pull request Dec 19, 2022
[skip ci]

## [4.30.0](v4.29.0...v4.30.0) (2022-12-19)

### Features

* auto register tsconfig-paths on env.EGG_TYPESCRIPT enable ([#254](#254)) ([6d75322](6d75322))
@github-actions
Copy link

🎉 This PR is included in version 4.30.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@@ -31,6 +31,10 @@ class EggCore extends KoaApplication {
*/
constructor(options = {}) {
options.baseDir = options.baseDir || process.cwd();
// auto require('tsconfig-paths/register') on typescript app
if (process.env.EGG_TYPESCRIPT === '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.

还需要增加一个 options 参数支持,生产环境不会使用 ts-node 运行,所以这里不会有 EGG_TYPESCRIPT,但是又需要 tsconfig-paths/register

Copy link
Member Author

Choose a reason for hiding this comment

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

#255 读取 package.json

@xsbear
Copy link

xsbear commented Apr 3, 2023

最近我们的egg项目升级依赖后,生产环境部署启动抛错:“Couldn't find tsconfig.json. tsconfig-paths will be skipped”,开始一直没找到什么原因,后来排查发现跟这个更新有关。我们的项目是ts的,也未用到 paths映射, 且生产构建后并没有把tsconfig.json 保留,但是 package.json 中的typescript属性还是为true,默认加载 tsconfig-paths后,就会导致抛 “Couldn't find tsconfig.json“ 的error。

解决方法倒不难,生产输出包中保留tsconfig.json (如果确实用到 paths映射,那还是必需的),或者改一下生产构建后的package.json,将typescript属性改为false。不知道其他用户是否有类似情况,建议可以做一个更新说明,或者给到一些生产环境构建输出的实践建议

@fengmk2
Copy link
Member Author

fengmk2 commented Apr 3, 2023

@xsbear 能否给一个可以复现的代码库和复现方式?

@xsbear
Copy link

xsbear commented Apr 5, 2023

@fengmk2 抱歉,我现在用 npm init egg --type=ts 初始化出来的已经是egg3.x版本了,跟我们项目使用的 egg2.x 已有差异,但tsc后去掉tsconfig.json,还是会报这个错,只不过3.x版本的example本身就用到了 paths映射,所以tsconfig.json在生产环境中也必需保留。代码库就不上传了。

在生产构建中不保留tsconfig.json,是我们这边的工程实践。我们这么做,也符合基本认知,生产环境用的是构建后的js,理论上是不需要tsconfig.json。但是这个更新后,如果package.json 中的typescript属性还是为true,则会默认加载 tsconfig-paths/register,没有tsconfig.json 自然会报错。解决方案我上面也说了,只是说这个更新确实对我们的生产环境启动产生了新的约束,如果有其他用户也碰到类似问题,可以参考一下,如果只是个例,也请忽略。

@fengmk2
Copy link
Member Author

fengmk2 commented Apr 6, 2023

@xsbear ts 项目基本都会用到 tsconfig-paths alias,编译出来的 js 文件对应的 require 代码也是依赖 tsconfig-paths 才能正常 require 的,除非是完全没有使用到 tsconfig-paths alias。

@xsbear
Copy link

xsbear commented Apr 6, 2023

@fengmk2 是的。只是我们的情况相对特殊,目前都没有用到 path alias,所以之前生产构建没有保留tsconfig.json 也没问题。这个版本更新后,就出问题了,好在现在已找到原因,解决方案也有,没事了哈。

@fengmk2
Copy link
Member Author

fengmk2 commented Apr 6, 2023

@xsbear 我还是加个修复判断吧,如果没有 tsconfig.json,不自动注入 tsconfig-paths

fengmk2 added a commit that referenced this pull request Apr 6, 2023
Avoid warning message: Couldn't find tsconfig.json. tsconfig-paths will be skipped

closes #254 (comment)
@fengmk2
Copy link
Member Author

fengmk2 commented Apr 6, 2023

#261

fengmk2 added a commit that referenced this pull request Apr 6, 2023
Avoid warning message: Couldn't find tsconfig.json. tsconfig-paths will be skipped

closes #254 (comment)
fengmk2 pushed a commit that referenced this pull request Apr 6, 2023
[skip ci]

## [5.3.1](v5.3.0...v5.3.1) (2023-04-06)

### Bug Fixes

* skip register tsconfig-paths if tsconfig.json not exists ([#261](#261)) ([24a0d64](24a0d64)), closes [/github.com//pull/254#issuecomment-1493713971](https://github.com/eggjs//github.com/eggjs/egg-core/pull/254/issues/issuecomment-1493713971)
@xsbear
Copy link

xsbear commented Apr 6, 2023

@fengmk2 4.x 版本会release吗?

@fengmk2
Copy link
Member Author

fengmk2 commented Apr 6, 2023

@xsbear 会的,我 pick 一下。

fengmk2 added a commit that referenced this pull request Apr 6, 2023
Avoid warning message: Couldn't find tsconfig.json. tsconfig-paths will be skipped

closes #254 (comment)

pick from #261
fengmk2 added a commit that referenced this pull request Apr 7, 2023
Avoid warning message: Couldn't find tsconfig.json. tsconfig-paths will be skipped

closes #254 (comment)

pick from #261
@fengmk2
Copy link
Member Author

fengmk2 commented Apr 7, 2023

@fengmk2 4.x 版本会release吗?

  • egg-core@4.30.2

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.

3 participants