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

fix: runtime should compat with old browsers #6478

Merged
merged 5 commits into from
Aug 24, 2023
Merged

Conversation

ClarkXia
Copy link
Collaborator

Compat runtime code with old browsers which depend on Object.assgin and Map.

@codecov-commenter
Copy link

codecov-commenter commented Aug 21, 2023

Codecov Report

Patch coverage: 59.67% and project coverage change: -0.05% ⚠️

Comparison is base (5fbf49a) 80.18% compared to head (647f89f) 80.13%.
Report is 11 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6478      +/-   ##
==========================================
- Coverage   80.18%   80.13%   -0.05%     
==========================================
  Files         240      240              
  Lines       21599    21617      +18     
  Branches     2655     2663       +8     
==========================================
+ Hits        17319    17323       +4     
- Misses       4236     4250      +14     
  Partials       44       44              
Files Changed Coverage Δ
packages/plugin-rax-compat/src/index.ts 94.89% <0.00%> (ø)
packages/runtime/src/Suspense.tsx 26.90% <7.69%> (-1.50%) ⬇️
packages/rax-compat/src/render.ts 74.24% <69.23%> (ø)
packages/rax-compat/src/container-root-map.ts 100.00% <100.00%> (ø)
packages/runtime/src/Document.tsx 91.91% <100.00%> (ø)

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ClarkXia ClarkXia added need review Need Review and removed need review Need Review labels Aug 21, 2023
@ClarkXia ClarkXia changed the base branch from master to release/next August 22, 2023 07:30
@ClarkXia ClarkXia added the need review Need Review label Aug 23, 2023
@@ -157,6 +166,6 @@ function Data(props) {
const data = useSuspenseData();

return (
<script dangerouslySetInnerHTML={{ __html: `if (!window.${LOADER}) { window.${LOADER} = new Map();} window.${LOADER}.set('${props.id}', ${JSON.stringify(data)})` }} />
<script dangerouslySetInnerHTML={{ __html: `!function(){if (!window.${LOADER}) { window['${LOADER}'] = {};} window['${LOADER}']['${props.id}'] = ${JSON.stringify(data)}}();` }} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里会是一个 breaking change 了, 假设有人调用了这个全局变量, 那么获取方式会发生变化

PS: 当然不应该调用这个私有变量

Copy link
Collaborator

Choose a reason for hiding this comment

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

var d = window['${LOADER}'] || {};
d['${props.id']=xxx;
window['${LOADER}'] = d;

Copy link
Collaborator

Choose a reason for hiding this comment

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

还有没有更少的版本

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

这里会是一个 breaking change 了, 假设有人调用了这个全局变量, 那么获取方式会发生变化

PS: 当然不应该调用这个私有变量

嗯 理论上不应该显示调用,后面可以单独沟通下重点已接入业务

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

var d = window['${LOADER}'] || {};
d['${props.id']=xxx;
window['${LOADER}'] = d;

window['${LOADER}'] = window['${LOADER}'] || {};
window['${LOADER}']['${props.id']=xxx;

对比这样没啥优势 - -

Copy link
Collaborator

Choose a reason for hiding this comment

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

优势: 少了一次 window['xxx']
劣势: 多了一个变量, 导致必须函数闭包一下

@wssgcg1213 wssgcg1213 merged commit d4904e9 into release/next Aug 24, 2023
6 checks passed
@wssgcg1213 wssgcg1213 deleted the fix/remove-map branch August 24, 2023 04:06
HomyeeKing pushed a commit to HomyeeKing/ice that referenced this pull request Sep 11, 2023
* fix: runtime should compat with old browsers

* fix: optimize code

* fix: lint

* chore: optimize code

* chore: optimize code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need review Need Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants