Skip to content

chore(dependency): 升级 rxjs 6#544

Open
chuan6 wants to merge 3 commits intoreleasefrom
chore/upgrade-rxjs6
Open

chore(dependency): 升级 rxjs 6#544
chuan6 wants to merge 3 commits intoreleasefrom
chore/upgrade-rxjs6

Conversation

@chuan6
Copy link
Copy Markdown
Contributor

@chuan6 chuan6 commented Sep 20, 2018 •
edited
Loading

/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
Copy Markdown

coveralls commented Sep 20, 2018 •
edited
Loading

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
Brooooooklyn
Brooooooklyn reviewed Sep 23, 2018
View reviewed changes
Comment thread src/rx.ts
@@ -0,0 +1,39 @@
export { defer } from 'rxjs/internal/observable/defer'
Copy link
Copy Markdown
Contributor

@Brooooooklyn Brooooooklyn Sep 23, 2018

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
Copy Markdown
Contributor Author

@chuan6 chuan6 Sep 23, 2018

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
Copy Markdown
Collaborator

@Saviio Saviio Sep 24, 2018

Choose a reason for hiding this comment

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

@Brooooooklyn 我们还没 webpack4 ...

Copy link
Copy Markdown
Collaborator

@Saviio Saviio Sep 24, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@Brooooooklyn Brooooooklyn Sep 25, 2018

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
Copy Markdown
Contributor Author

@chuan6 chuan6 Sep 26, 2018

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
Copy Markdown
Contributor Author

chuan6 commented Sep 26, 2018 •
edited
Loading

在上边提到的，原来打包结果大的原因是，没有配置好 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）。

@chuan6
chore(dependency): 升级 rxjs 6
0f60cfd
@chuan6
...尝试用一个 rx 模块把深模块引用做掉
d433513 
并不再在代码里区分 rxjs, rxjs/operators, rxjs/ajax 这样的子模块引用
（除了在 rx 模块中）。
@chuan6
...使用一个 reactivedb 无需摇树的版本
d9239de 
添加的 `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
chuan6
chuan6 commented Sep 27, 2018
View reviewed changes
Comment thread src/rx.ts
@@ -0,0 +1,41 @@
import 'rxjs/internal/symbol/observable'
Copy link
Copy Markdown
Contributor Author

@chuan6 chuan6 Sep 27, 2018 •
edited
Loading

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

Reviewers

@Saviio Saviio Awaiting requested review from Saviio

1 more reviewer

@Brooooooklyn Brooooooklyn Brooooooklyn left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@chuan6 chuan6

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@chuan6 @coveralls @Saviio @Brooooooklyn