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

Use common uuid generator everywhere #13255

Merged
merged 3 commits into from
Feb 20, 2024
Merged

Use common uuid generator everywhere #13255

merged 3 commits into from
Feb 20, 2024

Conversation

msujew
Copy link
Member

@msujew msujew commented Jan 10, 2024

What it does

Since merging #12935, we now have a native way of generating UUIDs instead of relying on the uuid package directly. With #13082, this generator has been used a lot in @theia/core. This change aligns the whole repo with this decision and removes all usage of the uuid package in Theia.

How to test

CI is green, manual smoke test is successful. Nothing should have changed in the behavior of the app.

Review checklist

Reminder for reviewers

@msujew msujew added the quality issues related to code and application quality label Jan 10, 2024
@msujew msujew requested a review from tsmaeder January 10, 2024 12:54
@msujew msujew force-pushed the msujew/use-better-uuid branch 2 times, most recently from 36aead1 to 8ed78e6 Compare January 11, 2024 12:00
@tsmaeder
Copy link
Contributor

@msujew we still seem to require the uuid types package in some package.json files: "@types/uuid": "^7.0.3"

@jfaltermeier
Copy link
Contributor

jfaltermeier commented Jan 15, 2024

In #13265 I am currently using uuid v5 to generate a hash that is an uuid.
My suggestion would be to add this uuid hash method in the theia common package as well like this: 63efe7a
However we would have to keep the uuid dependency on the core package then.
I will rebase my PR on this one once merged.
Would this be fine for you?

Copy link
Contributor

@tsmaeder tsmaeder left a comment

Choose a reason for hiding this comment

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

lgtm

@msujew
Copy link
Member Author

msujew commented Jan 18, 2024

@jfaltermeier I would recommend the reverse; Please merge your PR first and then I'll rebase :)

@jfaltermeier
Copy link
Contributor

@msujew Thanks, my PR is merged now

@JonasHelming
Copy link
Contributor

@msujew I think this could be merged now

This was referenced Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quality issues related to code and application quality
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants