Closed Ryxias closed 5 months ago
Code-wise it looks good to me, design-wise I have a question/scenario:
I don't think this would break anyone's code and it definitely makes sense to emit an event in those spots. But I wonder if someone would still be able to differentiate whether the item was spawning for the first time or being rehydrated or something? If not, I could see that causing bugs. Maybe the container it is spawned into could be passed as an argument? (the room, npc, item container, etc.)
That is a very good point @seanohue; this pattern could cause multiple spawn
events to be emitted on any single creation attempt.
A workaround would be to detect if an item has been spawned already before emitting the event... but that seems kind of, inelegant. I'll think about it
Maybe something like item.emit('spawn', room)
or item.emit('spawn', item);
so that one could check against where it is spawned. That said, such a change could always be made later in case there is something that breaks or is otherwise backwards-incompatible.
Has this been tested in the wild yet?
I've tested this and I'm not encountering duplicate spawn
events per item. All of these new events are emitted only during hydration, which only happens when:
An item won't be spawned/hydrated in multiple places and the event won't be emitted multiple times per item, as each spawn/hydrate creates a unique item.
Separately, there's no way to differentiate between an item that's spawned for the first time and an item that was simply rehydrated. If a player possesses an item (either in their inventory or equipped) that later respawns back in its original area (thus creating a second copy of the item), a spawn
event will fire when the player logs in and hydrates their inventory or equipment while they possess the original item, and a spawn
event will fire at the time when the replacement item respawns at its original location. Those are two separate items, however, so each should trigger their own Item#spawn
event.
These changes look good to me!
Came across this again while going through open, public PRs that I'm mentioned in. This looks ready to merge but unfortunately I don't have merge permissions at this point.
Hoping to draw attention to it with a comment.
To be consistent with the
@event Item#spawn
event that is fired inRoom#spawnItem
Addresses #73