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

Require email for attendee #344

Closed
midzdotdev opened this issue Jan 21, 2022 · 8 comments
Closed

Require email for attendee #344

midzdotdev opened this issue Jan 21, 2022 · 8 comments

Comments

@midzdotdev
Copy link

Just tried to submit a PR and failed.

I've tried to create events with attendees who have no email and it this library throws. I feel that the types for ICalInternalAttendeeData and other relevant types should indeed require email.

@midzdotdev
Copy link
Author

It's when the Attendee is stringified and throws for a falsy email on this line

I think allowing constructing an attendee with a falsy email is confusing. I suppose this would also apply to the email setter
accepting falsy values too.

@midzdotdev
Copy link
Author

Awesome library btw! ❤️

@sebbo2002
Copy link
Owner

In fact this is true for all mandatory fields in ical-generator. The reason for this is that ical-generator has always allowed you to use the setters, in this case email() for example, and you are not required to include all the data with the constructor. This is currently absolutely valid code, for example:

const attendee = event.createAttendee();
attendee.email('foo@bar.com');

I understand that this is a bit weird, but I hope that the exceptions is meaningful enough. For the next breaking version it is already planned that at least the mandatory fields have to be passed to achieve some security here.

@midzdotdev
Copy link
Author

Yes thank makes perfect sense. I'm personally a big fan of immutable data structures and so recreating when a change is needed. In this case, there can never be an event or an attendee with a state which cannot be stringified to iCalendar.

It sounds like your moving towards that a little bit in the next breaking version you mentioned.

I wish you the best with this :)

@sebbo2002
Copy link
Owner

I hope so, thank you very much. I would close the ticket for now, okay?

@sebbo2002 sebbo2002 reopened this Sep 16, 2023
@sebbo2002 sebbo2002 mentioned this issue Oct 19, 2023
github-actions bot pushed a commit that referenced this issue Oct 19, 2023
# [6.0.0-develop.1](v5.0.2-develop.2...v6.0.0-develop.1) (2023-10-19)

### Features

* Ensure Calendar is renderable all the time ([f1328a3](f1328a3)), closes [#344](#344)
* Remove `save()`, `saveSync()`, `serve()`, `toBlob()`, `toURL()` ([b6bea66](b6bea66)), closes [#478](#478)

### BREAKING CHANGES

* `Alarm.trigger` now defaults to 10min before event, `Alarm.type` now defaults to `display`, `Alarm.interval()` got removed, use `Alarm.repeat()` instead, `Alarm.repeat()` now gives/takes an object instead of a number, `Attendee.email` can’t be `null | undefined`, `Category.name` can’t be `null | undefined`, `Event.start` now defaults to now (`new Date()`). For details and examples checkout the migration guide at https://github.com/sebbo2002/ical-generator/wiki/Migration-Guide:-v5-%E2%86%92-v6
* The `save()`, `saveSync()`, `serve()`, `toBlob()` and `toURL()` methods of the ICalCalendar class have been removed. Please use the `toString()` method to generate the ical string and proceed from there.
@sebbo2002
Copy link
Owner

🎉 This issue has been resolved in version 6.0.0-develop.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@sebbo2002
Copy link
Owner

I just pushed a preview of ical-generator v6 to develop, which should fix this issue. I would be happy if you could test the new version and give me some feedback. You can install the version with npm i ical-generator@next. All changes in this release can be found here.

github-actions bot pushed a commit that referenced this issue Oct 25, 2023
# [6.0.0](v5.0.1...v6.0.0) (2023-10-25)

### Bug Fixes

* add `browser` field to `package.json` ([7db4e32](7db4e32))

### Features

* Enable npm provenance ([87d173a](87d173a))
* Enable npm provenance ([ccba971](ccba971))
* Ensure Calendar is renderable all the time ([f1328a3](f1328a3)), closes [#344](#344)
* Remove `save()`, `saveSync()`, `serve()`, `toBlob()`, `toURL()` ([b6bea66](b6bea66)), closes [#478](#478)

### Reverts

* Revert "ci: Downgrade is-semantic-release till it's fixed" ([91c2ab5](91c2ab5))

### BREAKING CHANGES

* `Alarm.trigger` now defaults to 10min before event, `Alarm.type` now defaults to `display`, `Alarm.interval()` got removed, use `Alarm.repeat()` instead, `Alarm.repeat()` now gives/takes an object instead of a number, `Attendee.email` can’t be `null | undefined`, `Category.name` can’t be `null | undefined`, `Event.start` now defaults to now (`new Date()`). For details and examples checkout the migration guide at https://github.com/sebbo2002/ical-generator/wiki/Migration-Guide:-v5-%E2%86%92-v6
* The `save()`, `saveSync()`, `serve()`, `toBlob()` and `toURL()` methods of the ICalCalendar class have been removed. Please use the `toString()` method to generate the ical string and proceed from there.
@sebbo2002
Copy link
Owner

🎉 This issue has been resolved in version 6.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

No branches or pull requests

2 participants