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 UUID v4 for the ID property and remove the domain property #215

Closed
wants to merge 2 commits into from
Closed

Use UUID v4 for the ID property and remove the domain property #215

wants to merge 2 commits into from

Conversation

gprst
Copy link

@gprst gprst commented Oct 1, 2020

Resolves #203.

  • UUID v4 support: I chose uuid-random over more popular packages (such as uuid) because its unpacked size is much smaller, and it only implements the feature we need here — random (cryptographically secure) UUID v4 generation.

  • domain property removal: as this property isn't used anywhere anymore, it is completely wiped from the project.

With this, the UID property of an event is now a UUID, rather than an ID with @somedomain appended.

@gprst gprst changed the title feat(event): Use UUID v4 for the ID property Use UUID v4 for the ID property and remove the domain property Oct 3, 2020
@gprst
Copy link
Author

gprst commented Oct 18, 2020

Hi, out of curiosity, is there any preliminary thing I'd need to fulfil before this PR gets merged ?

@sebbo2002
Copy link
Owner

@gprst No, this PR is currently lying around until several breaking changes for a new major version have come together. I don't see this PR as so important that it should be merged immediately. So it's not for you to do, don't worry. ☺️

@sebbo2002 sebbo2002 added this to the v2.0.0 🎉 milestone Feb 5, 2021
@sebbo2002 sebbo2002 closed this in a4c19cc Mar 5, 2021
sebbo2002 pushed a commit that referenced this pull request Mar 5, 2021
# [1.3.0-develop.2](v1.3.0-develop.1...v1.3.0-develop.2) (2021-03-05)

### Features

* **Events:** Use uuid-random for random UUIDs (close [#215](#215)) ([a4c19cc](a4c19cc))
@sebbo2002
Copy link
Owner

🎉 This issue has been resolved in version 1.3.0-develop.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

This was referenced Apr 10, 2021
@gprst
Copy link
Author

gprst commented Apr 12, 2021

@sebbo2002 It appears you've just taken my commits and published them in your name. That's not nice

@sebbo2002
Copy link
Owner

@gprst Fortunately, it only looks like this at first glance.

Unfortunately, the pull request could no longer be merged without major conflicts after the typescript refactoring, so I decided to go this route.

Of course, I could have written to you again to update the PR, but honestly: I assumed that no one would feel offended, because in the end there was only one import and one uuid() left from your pull request, as the removal of domain() was done for other reasons during the typescript refactoring without having your pull request in mind. Feed free to double check the commits, I linked them above.

Of course, you are free to open a pull request and comment on the use of uuid-random, so you are welcome to point out that this piece of code originally came from you. But if you do, it's please also link to this issue, because I don't want to be accused of stealing code.

@gprst
Copy link
Author

gprst commented Apr 13, 2021

Okay, thank you for taking the time to explain me what lead this PR to be closed and the feature still be added. I should have taken the time to look at the differences between the code you wrote and mine. My appologies for the quick accusations.

sebbo2002 pushed a commit that referenced this pull request Apr 28, 2021
# [2.0.0](v1.15.4...v2.0.0) (2021-04-28)

### Bug Fixes

* **package.json:** add temporary version ([0bc117e](0bc117e))
* Allow to set null values within object constructors ([8b87183](8b87183))
* **deps:** Also define libs as devDependency for tests ([c04ae32](c04ae32))
* **deps:** Define supported libs as peerDependencies ([84e2784](84e2784))
* Make peer dependencies optional ([b384ac7](b384ac7)), closes [#244](#244)
* **Tools:** Prevent formatDate() from using global timezones prefixed with a slash ([85ab7b2](85ab7b2))
* **deps:** Put necessary typings in peerDependencies as well :/ ([14f0f43](14f0f43))
* **Event:** Remove `moment` dependency in constructor ([8331d4c](8331d4c)), closes [#234](#234)

### Code Refactoring

* **Calendar:** Remove moment.Duration from `ttl()` method ([c6ccd12](c6ccd12))
* Update error URLs ([2aedf55](2aedf55))

### Features

* **Event:** Add `priority()` method ([247039f](247039f)), closes [#163](#163)
* **Attendee:** Add `x()` method for custom attributes ([5d9d686](5d9d686)), closes [#183](#183)
* **Calendar:** add new clear method ([1ebefcb](1ebefcb)), closes [#188](#188)
* Add ReleaseBot ([2fba164](2fba164))
* **Calendar:** Add support for external VTimezone generator ([f4bc8e0](f4bc8e0)), closes [#122](#122)
* **Event:** Allow `X-APPLE-STRUCTURED-LOCATION` without address ([4e63e29](4e63e29)), closes [#236](#236)
* **Event:** Make organizer.email optional ([8450492](8450492)), closes [#137](#137)
* **Event:** Merge `location()`, `appleLocation()` and `geo()` ([62c1516](62c1516)), closes [#187](#187)
* Merge event's `description()` and `htmlDescription()` ([ce537f8](ce537f8))
* Support moment.js, Day.js and Luxon ([#91](#91), BREAKING CHANGE) ([6db24ee](6db24ee))
* **Event:** Support RRule objects and raw strings in `repeating()` ([4436785](4436785)), closes [#190](#190)
* Updated the entire codebase to Typescript ([d013dc0](d013dc0))
* **Events:** Use uuid-random for random UUIDs (close [#215](#215)) ([a4c19cc](a4c19cc))

### BREAKING CHANGES

* Some error messages changed, so if you check for error , please double check them now.
* `htmlDescription()` was removed, use `description()` instead.
* **Calendar:** `ttl()` will now return a number, not a `moment.Duration`. You can still use `moment.Duration` to set the `ttl` value.
* **Event:** `geo()` and `appleLocation()` are not available anymore, use `location()` instead and pass an location object (with title, radius, etc.)
* **Calendar:** Calendar's `clear()` method is a completely new implementation and, unlike previous versions, will not reset metadata such as `name` or `prodId`. Only the events will be removed
@sebbo2002
Copy link
Owner

🎉 This issue has been resolved in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use UUIDs for Event UIDs
2 participants