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

Add copy event id/hash button. #114

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Nufflee
Copy link
Contributor

@Nufflee Nufflee commented Jul 19, 2019

Closes #104.

package.json Outdated Show resolved Hide resolved
ts/util/copyToClipboard.ts Show resolved Hide resolved
ts/Event.ts Outdated Show resolved Hide resolved
@Nufflee Nufflee requested a review from rexim February 10, 2020 19:11
@@ -67,8 +67,8 @@ export default class EventsForDay implements UiComponent {
.map(
(e) => new Event(
this._state.eventPatches
? new dto.PatchedEvent(e, this._state.eventPatches[e.datetime.utc().unix()])
: e,
? new dto.PatchedEvent(e as dto.Event, this._state.eventPatches[e.datetime.utc().unix()])

Choose a reason for hiding this comment

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

I thought I'd chime in since I was watching @Nufflee work on this on stream. e as dto.Event is an unchecked cast and it is an invalid cast in this particular case. e is a plain javascript object that is defined in either of the returns on lines 41 or 57. Casting this object to dto.Event does not convert it into an instance of the class. However, this is an interesting situation because the application works correctly despite this bug. As passed to the constructor of PatchedEvent, the object instance is missing the timestamp() method and calling instance of Event on it would return false. What "fixes" this, is that the PatchedEvent dto class inherits from the Event dto class. The super call uses the properties of the passed in event object to instantiate a correct Event dto object which PatchedEvent inherits from, so the object from the invalid cast is never used anywhere else. This just so happens to work because the property names in the Event dto class happily line up with the property names in the returns in EventsForDay.ts (again, lines 41 and 57), but this is just "coincidence" - there is no type checking here.

Copy link
Contributor Author

@Nufflee Nufflee Feb 10, 2020

Choose a reason for hiding this comment

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

I see and I think I understand what you're talking about but what's the solution your suggesting? Actually creating a new instance instead of casting?

Choose a reason for hiding this comment

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

If I may also add my opinion to this: by changing some of the dtos from interfaces to classes, the code is now written in two different paradigms: structural typing and OOP with prototype-based inheritance. Before, it was only structural typing, which I would argue is a better fit for the use case and there is no need to introduce OOP here, if patching events was handled like so:

type event_t = {
    datetime: number,
    title: string,
    description: string,
    url: string,
    channel: string
}

type event_patch_t = Partial<Omit<event_t, "datetime">>;

function patch_event(
    event: event_t,
    patch: event_patch_t): event_t {
    return { ...event, ...patch };
}

let abcd_event: event_t = {
    datetime: -1,
    title: 'abcd',
    description: 'description',
    url: 'https://google.com',
    channel: 'abcd'
}

console.log(patch_event(abcd_event, { title: 'patched' }));

However, obviously, this is not my codebase, so please treat this as a suggestion.

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.

Inconvenient to copy Event ids to clipboard
3 participants