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: repeat runtime plugin key #11590

Merged
merged 4 commits into from
Sep 26, 2023

Conversation

PopperLi
Copy link
Contributor

@PopperLi PopperLi commented Aug 31, 2023

基于某些特殊场景我需要通过插件执行addRuntimePluginKey,处于umi限制我无法知道哪些key已经被添加,所以我可能会重复add,但最终产物中会产生重复的key,例如两个layout、两个qiankun等等,所以我想它需要被去重。

@vercel
Copy link

vercel bot commented Aug 31, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
umi ⬜️ Ignored (Inspect) Visit Preview Sep 5, 2023 6:35am

@fz6m
Copy link
Contributor

fz6m commented Sep 1, 2023

这个地方建议变成 lodash.uniq(result).length !== result.length 就:

  throw new Error(`The plugin key cannot be duplicated. (${result.join(', ')})`)

并告知用户重复添加 key 了,因为重复添加 key 本来就是不合理的行为,应该避免明确告诉用户不要做错误的行为而不是容许。

@PopperLi
Copy link
Contributor Author

PopperLi commented Sep 1, 2023

这个地方建议变成 lodash.uniq(result).length !== result.length 就:

  throw new Error(`The plugin key cannot be duplicated. (${result.join(', ')})`)

并告知用户重复添加 key 了,因为重复添加 key 本来就是不合理的行为,应该避免明确告诉用户不要做错误的行为而不是容许。

感谢回复

我先明确说一下我的应用场景,我需要N多个微前端项目公用一套运行时配置,但根据#11384 (comment) 回复道目前不支持一个export default导出所有运行时配置。
所以我只能另辟蹊径,由一个依赖(假设包名为@app/runtime)统一导出所有运行时配置,再由项目中的app.ts统一导出所有项,例如:

export * from '@app/runtime';

但我发现这么做的话某些插件例如initialState,无法检测到我有导出getInitialState这个方法,因为它是调用api.appData.appJS?.exports判断是否有导出。所以我就从api.appData.appJS.exports下手,由于运行时配置可能会随时变更,又不想维护方法的同时再维护一遍appJS.exports。我就针对性的对我的依赖包进行了代码扫描,动态解析出导出的方法名,然后写到appJS.exports里。

后来又发现没有被注册到getValidKeys里的方法都会被判定失效,才又执行了addRuntimePluginKey,最后发现是能用了,但是会有重复key的产生

最后,我才提个这个pr。

所以如果直接抛错,我就需要再维护一份RuntimePluginKey,这可能会增加我的维护负担,我需要根据@app/runtime导出了哪些方法手动去配置appJS.exports

如果非要提示避免重复key的产生,我希望能是warning而不是直接error

@fz6m
Copy link
Contributor

fz6m commented Sep 2, 2023

重复 key 本身是一种错误的使用方式,应该在使用时就避免添加重复的 key ,而不是内部明知有错误还容许错误继续存在。

@PopperLi
Copy link
Contributor Author

PopperLi commented Sep 4, 2023

重复 key 本身是一种错误的使用方式,应该在使用时就避免添加重复的 key ,而不是内部明知有错误还容许错误继续存在。

那换成警告可行吗?

@fz6m
Copy link
Contributor

fz6m commented Sep 4, 2023

我的建议是不能继续保留错误的行为,因为当报错出现了,用户就知道要对自己的插件做一定的调整,改成正确的行为,只要不报错,一切都是正确的,而不是用户做了错误的行为还可以运行,这样只会变成用户不知道应该添加哪些 key ,最后每个人就会把所有的 key 都 copy 一份,每个人的代码都很混乱。

@PopperLi
Copy link
Contributor Author

PopperLi commented Sep 4, 2023

我的建议是不能继续保留错误的行为,因为当报错出现了,用户就知道要对自己的插件做一定的调整,改成正确的行为,只要不报错,一切都是正确的,而不是用户做了错误的行为还可以运行,这样只会变成用户不知道应该添加哪些 key ,最后每个人就会把所有的 key 都 copy 一份,每个人的代码都很混乱。

改过了

@codecov
Copy link

codecov bot commented Sep 5, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.03% ⚠️

Comparison is base (344ff1a) 29.01% compared to head (00501b3) 28.99%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11590      +/-   ##
==========================================
- Coverage   29.01%   28.99%   -0.03%     
==========================================
  Files         484      484              
  Lines       14710    14721      +11     
  Branches     3476     3479       +3     
==========================================
  Hits         4268     4268              
- Misses       9686     9696      +10     
- Partials      756      757       +1     
Files Changed Coverage Δ
...kages/preset-umi/src/features/tmpFiles/tmpFiles.ts 0.00% <0.00%> (ø)

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

@sorrycc sorrycc merged commit 33abef0 into umijs:master Sep 26, 2023
23 checks passed
@PopperLi PopperLi deleted the fix/repeat-runtime-plugin-key branch November 13, 2023 09:39
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.

3 participants