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: renderClient with callback #4979

Merged
merged 10 commits into from Jul 27, 2020
Merged

feat: renderClient with callback #4979

merged 10 commits into from Jul 27, 2020

Conversation

ycjcl868
Copy link
Contributor

@ycjcl868 ycjcl868 commented Jul 6, 2020

For umijs/plugins#293

Checklist
  • npm test passes
  • tests are included
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

@ycjcl868 ycjcl868 changed the title feat: render with callback feat: renderClient with callback Jul 6, 2020
@ycjcl868 ycjcl868 requested review from sorrycc and kuitos and removed request for sorrycc July 6, 2020 09:42
@codecov
Copy link

codecov bot commented Jul 6, 2020

Codecov Report

Merging #4979 into master will increase coverage by 0.21%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4979      +/-   ##
==========================================
+ Coverage   82.84%   83.06%   +0.21%     
==========================================
  Files         153      153              
  Lines        3381     3383       +2     
  Branches      888      904      +16     
==========================================
+ Hits         2801     2810       +9     
+ Misses        573      565       -8     
- Partials        7        8       +1     
Impacted Files Coverage Δ
...s/renderer-react/src/renderClient/renderClient.tsx 72.72% <50.00%> (+9.93%) ⬆️
...ges/renderer-mpa/src/renderClient/renderClient.tsx 95.23% <100.00%> (+15.23%) ⬆️
packages/core/src/Config/Config.ts 61.78% <0.00%> (ø)

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 0e52bcd...441165b. Read the comment docs.

Copy link
Member

@kuitos kuitos left a comment

Choose a reason for hiding this comment

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

LGTM

@sorrycc
Copy link
Member

sorrycc commented Jul 7, 2020

插件里怎么用?oldRender({ callback }) ? 如果多套了几层插件会不会能传下去?

@ycjcl868
Copy link
Contributor Author

ycjcl868 commented Jul 7, 2020

插件里怎么用?oldRender({ callback }) ? 如果多套了几层插件会不会能传下去?

会执行,compose 好像是依次执行

@sorrycc
Copy link
Member

sorrycc commented Jul 7, 2020

插件里怎么用?oldRender({ callback }) ? 如果多套了几层插件会不会能传下去?

会执行,compose 好像是依次执行

看写法应该不会,加个用例试试。

@kuitos
Copy link
Member

kuitos commented Jul 7, 2020

插件里怎么用?oldRender({ callback }) ? 如果多套了几层插件会不会能传下去?

确实会有这个问题,oldRender 如果不是 render 插件列表模板的那个,是拿不到这个入参的,除非这么改一下 20addc3#diff-769d306c38234a69b1fb8eb7bad75b92R23

@PeachScript
Copy link
Member

@kuitos @ycjcl868 实测参数传递确实会有问题,我尝试实现 #5020 ,发现 render 无论如何都只能拿到 callback,拿不到 qiankun 注入的 props

@ycjcl868
Copy link
Contributor Author

/rebase

@PeachScript
Copy link
Member

@kuitos @ycjcl868 实测参数传递确实会有问题,我尝试实现 #5020 ,发现 render 无论如何都只能拿到 callback,拿不到 qiankun 注入的 props

我验证的方式有问题,走 defer 传递验证参数可传递到 renderClient,可以拿到注入的 props

@ycjcl868
Copy link
Contributor Author

验证 ok,可以用,我 merge 下 master

return renderClient({
...opts,
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/umijs/umi/pull/5067/files 里的变更好像重复了

...opts 挪到最下面?相对于允许覆盖默认配置

Copy link
Contributor Author

Choose a reason for hiding this comment

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

不建议覆盖默认的值,...opts 问题排查起来比较麻烦

Copy link
Contributor Author

@ycjcl868 ycjcl868 left a comment

Choose a reason for hiding this comment

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

LGTM

@ycjcl868 ycjcl868 requested a review from sorrycc July 27, 2020 03:19
@kuitos kuitos merged commit 677cd21 into master Jul 27, 2020
@delete-merged-branch delete-merged-branch bot deleted the feat-render-callback branch July 27, 2020 03:22
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.

None yet

4 participants