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: remove shortid dependency #923

Merged
merged 4 commits into from
Apr 29, 2022
Merged

feat: remove shortid dependency #923

merged 4 commits into from
Apr 29, 2022

Conversation

dqhl76
Copy link
Contributor

@dqhl76 dqhl76 commented Apr 27, 2022

Types

  • 🎉 New Features

Background or solution

Use nanoid to replace shortid

close #882

Changelog

replace shortid with nanoid dependency

@codecov
Copy link

codecov bot commented Apr 27, 2022

Codecov Report

Merging #923 (9a4ea65) into main (3340fa8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #923   +/-   ##
=======================================
  Coverage   58.48%   58.48%           
=======================================
  Files        1220     1220           
  Lines       75099    75099           
  Branches    15616    15616           
=======================================
+ Hits        43923    43924    +1     
+ Misses      28416    28415    -1     
  Partials     2760     2760           
Impacted Files Coverage Δ
...kages/connection/src/browser/ws-channel-handler.ts 78.57% <100.00%> (ø)
packages/utils/src/uuid.ts 100.00% <100.00%> (ø)
packages/core-common/src/node/port.ts 43.93% <0.00%> (-3.04%) ⬇️
...ages/file-service/src/node/file-service-watcher.ts 67.03% <0.00%> (+1.67%) ⬆️

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 3340fa8...9a4ea65. Read the comment docs.

@dqhl76
Copy link
Contributor Author

dqhl76 commented Apr 27, 2022

image

这个报错似乎是和我的修改无关,其似乎于快捷键功能相关

@bytemain
Copy link
Member

@erha19 #784 不是还没合吗,这个 pr 合进去之后你那边又冲突了

bytemain
bytemain previously approved these changes Apr 27, 2022
@erha19
Copy link
Member

erha19 commented Apr 27, 2022

@erha19 #784 不是还没合吗,这个 pr 合进去之后你那边又冲突了

相关的冲突我后面再另外一个 PR 处理吧

@dqhl76
Copy link
Contributor Author

dqhl76 commented Apr 27, 2022

@bytemain 请问刚才是重新运行了一下CI,它就不报错了吗

@bytemain
Copy link
Member

@bytemain 请问刚才是重新运行了一下CI,它就不报错了吗

是的,那个 case 可能有点问题,已经记录在了 #849

@dqhl76
Copy link
Contributor Author

dqhl76 commented Apr 27, 2022

@bytemain 请问刚才是重新运行了一下CI,它就不报错了吗

是的,那个 case 可能有点问题,已经记录在了 #849

好的,明白了🚀

packages/core-common/src/uuid.ts Outdated Show resolved Hide resolved
packages/connection/package.json Outdated Show resolved Hide resolved
@bytemain
Copy link
Member

需要解决一下冲突:

首先把 opensumi/core:main 设置为你的上游。

git remote add upstream https://github.com/opensumi/core.git
git pull upstream main --rebase

然后就可以解决冲突了

@dqhl76
Copy link
Contributor Author

dqhl76 commented Apr 27, 2022

需要解决一下冲突:

首先把 opensumi/core:main 设置为你的上游。

git remote add upstream https://github.com/opensumi/core.git
git pull upstream main --rebase

然后就可以解决冲突了

好的,我已经解决了冲突,感谢您的指导,剩下我需要做的是:

  1. 去除connection中的shortid依赖(修改package)
  2. 将原本位于packages/core-common的nanoid,转移到packages/utils

@dqhl76
Copy link
Contributor Author

dqhl76 commented Apr 28, 2022

CI/coverage(12.x)这个似乎又是和我本次PR无关的问题失败了,我在我fork的仓库中跑了一下是没问题的,但我没有办法让它重新在这个仓库重新运行, @erha19 @bytemain 可以帮我重新跑一下这个CI吗

@erha19 erha19 merged commit 9938d97 into opensumi:main Apr 29, 2022
@dqhl76 dqhl76 deleted the feat/remove-shortid-dependency branch April 29, 2022 14:12
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.

[FEATURE] remove shortid dependency
3 participants