Skip to content
This repository has been archived by the owner on May 17, 2019. It is now read-only.

Support * event type for mapping/handling #9

Merged
merged 4 commits into from
Nov 14, 2017

Conversation

ganemone
Copy link
Contributor

@ganemone ganemone commented Nov 14, 2017

Fixes #10

lhorie
lhorie previously approved these changes Nov 14, 2017
const globalMappers = this.mappers[globalEventType] || [];
const globalHandlers = this.handlers[globalEventType] || [];
const mappers = (this.mappers[type] || []).concat(globalMappers);
const handlers = (this.handlers[type] || []).concat(globalHandlers);
const event = {
type,
payload: mappers.reduce((payload, mapper) => {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we previously had an implicit wildcard match in getArgs below, which mapped to all key-less types. Given this change, that would no longer be supported (a la 'all key-less types are handled together'). Is that something we wanted to support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the intended behavior of that was to have * behave as a wildcard. I don't think there is any value in supporting events with no type.

@ganemone ganemone self-assigned this Nov 14, 2017
Copy link
Contributor

@KevinGrandon KevinGrandon left a comment

Choose a reason for hiding this comment

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

Listening on * seems like it could be a footgun to me. If we're sure though...

@AlexMSmithCA
Copy link
Member

AlexMSmithCA commented Nov 14, 2017

I agree it may potentially be misleading for folks (Cool! I can wildcard anything... myPlugin::*).I do think the use-case itself seems reasonable. As an alternative, I could see expanding the surface area of the API with something like .onAny or .onGlobal

@ganemone
Copy link
Contributor Author

Listening on * seems like it could be a footgun to me. If we're sure though...

It definitely is. tbh, I would consider having this be an undocumented feature. We are only adding it because we need it to accomplish something somewhat uber specific.

@ganemone
Copy link
Contributor Author

!merge

@old-fusion-bot old-fusion-bot bot merged commit 94e2f6d into fusionjs:master Nov 14, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants