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

chore(dependency): 升级 rxjs 6 #544

Open
wants to merge 3 commits into
base: release
Choose a base branch
from
Open

Conversation

chuan6
Copy link
Contributor

@chuan6 chuan6 commented Sep 20, 2018

/cc @aicest

TODOs:

@chuan6 chuan6 self-assigned this Sep 20, 2018
@chuan6 chuan6 requested a review from Saviio September 20, 2018 11:43
@coveralls
Copy link

coveralls commented Sep 20, 2018

Coverage Status

Coverage increased (+0.02%) to 95.951% when pulling d9239de on chore/upgrade-rxjs6 into 4e9ab51 on release.

@chuan6 chuan6 force-pushed the chore/upgrade-rxjs6 branch from e525fa0 to 63d304a Compare September 22, 2018 03:14
@@ -0,0 +1,39 @@
export { defer } from 'rxjs/internal/observable/defer'
Copy link
Contributor

Choose a reason for hiding this comment

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

too hack, 直接引用就好了,webpack4 会帮你摇树

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Brooooooklyn 发现这样包反而大了,不知道为什么,我今天有空再看一下。我觉得到最后这些东西肯定是在一个 'rxjs' 上引最舒服,所以想着可以有这么一个 rx 模块把具体来源 rxjs, rxjs/operators, rxjs/ajax 这些封装掉。所以提了第二个 commit 做了这样的修改。还没想清楚,这样的形式好不好。你有建议吗?谢谢!

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Brooooooklyn 我们还没 webpack4 ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

另外我觉得我们可以用 esm 了

Copy link
Contributor

Choose a reason for hiding this comment

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

没有用 webpack4 的话可以用 ts-import-plugin 去处理一下,我觉得源码内保持 rxjs 的原始引用方式比较好

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Saviio 记得看过 dan 最近对发布 esm 的这串评论:https://twitter.com/dan_abramov/status/1009185305117843456 @~@

@chuan6
Copy link
Contributor Author

chuan6 commented Sep 26, 2018

在上边提到的,原来打包结果大的原因是,没有配置好 tree shake 机制的情况下,虽然本项目使用了深引用来手动挑出需要的 rxjs 模块,但如果依赖的 reactivedb 用的是需要有匹配的 tree shake 机制才能被缩小的 rxjs@6 推荐引用写法(如:import ... from 'rxjs', import ... from 'rxjs/operators'),就还是会把整个 rxjs 依赖都加进来(如:var operators = Object.freeze({ ...所有操作符... }))。

为了验证这个想法,在 reactivedb 开出一个实验分支,其内也使用深引用来手动挑出需要的 rxjs 模块。并发版 0.11.1-alpha.0-rxnotreeshake。得到了预期结果:打包得出的大小正常了(237.58KB)。

并不再在代码里区分 rxjs, rxjs/operators, rxjs/ajax 这样的子模块引用
(除了在 rx 模块中)。
添加的 `import 'rxjs/internal/symbol/observable` 是用来保证在
reactivedb 的 rxjs 依赖完全清瘦的情况下,SymbolConstructor 上的
observable 字段被挂上,令 ts 编译通过。
@chuan6 chuan6 force-pushed the chore/upgrade-rxjs6 branch from 6687571 to d9239de Compare September 27, 2018 01:51
@@ -0,0 +1,41 @@
import 'rxjs/internal/symbol/observable'
Copy link
Contributor Author

@chuan6 chuan6 Sep 27, 2018

Choose a reason for hiding this comment

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

@Brooooooklyn 不知道你有没有看到过需要这句话,引入将 observable 挂在 SymbolConstructor 类型上的副作用,才能让某些操作符或者构造函数编译成功的?我还没追踪,打算再看一下。

不加这句话,我遇到了 Property 'observable' does not exist on type 'SymbolConstructor' 这个报错。

/cc @Saviio

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