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

Symbols fix #211

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

Symbols fix #211

wants to merge 2 commits into from

Conversation

Profesor08
Copy link

Description

Symbol message is converted to string, this behavior make uselles using symbols. Because symbols are not strings, and every symbol is unique Symbol("message") !== Symbol("message").

What is the purpose of this pull request?

  • Bug fix

Reproduction

Next code will call func1 twice. This is unexpecte behavior, func1 must be called only once.

        var MESSAGE1 = Symbol("MESSAGE");
        var MESSAGE2 = Symbol("MESSAGE");

        var func1 = function (msg) {
            return undefined;
        };

        PubSub.subscribe(MESSAGE1, func1);

        PubSub.publish(MESSAGE1);
        PubSub.publish(MESSAGE2);

@Profesor08
Copy link
Author

@mroderick some attention please

@fezhengjin
Copy link

@mroderick same to me

@verekia
Copy link

verekia commented Sep 12, 2022

Is there something we can help with to get this merged?

Maybe removing 'MESSAGE' from the test and 'MY_TOPIC' from the docs?

https://github.com/mroderick/PubSubJS/blob/master/README.md?plain=1#L217

I added a comment on the PR that introduced Symbol support: #141 (comment)

@mroderick
Copy link
Owner

This looks good to me. Is there anything you'd add/change @verekia ?

@verekia
Copy link

verekia commented Dec 16, 2022

Looks good to me too :)

@dikaso
Copy link

dikaso commented Jun 30, 2023

Any news here? Are you going to release it anytime soon?

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.

5 participants