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

Support es6 symbols in proptype #4917

Closed
dashed opened this issue Sep 20, 2015 · 13 comments
Closed

Support es6 symbols in proptype #4917

dashed opened this issue Sep 20, 2015 · 13 comments

Comments

@dashed
Copy link

dashed commented Sep 20, 2015

Would you be open to support es6 symbols in proptype?

At the moment I'm testing them as typeof Symbol() === 'symbol' in custom proptype functions.

@steve-taylor
Copy link

Here's what I use (not sure if it's the right approach):

function symbol(props, propName, componentName) {
    const prop = props[propName];
    if (prop && (typeof prop !== 'symbol')) {
        return new Error(`[${componentName}]: Expected property ${propName} to be a Symbol, but its actual type is ${typeof prop}.`);
    }
}

symbol.isRequired = function(props, propName, componentName) {
    return props[propName] ?
           symbol(props, propName, componentName) :
           new Error(`[${componentName}]: Property ${propName} is required.`);
};

export default symbol

@zpao
Copy link
Member

zpao commented Sep 21, 2015

We've generally said no to a lot of custom proptypes checks but this one feels like it's probably ok. In theory it's a 1 line change. However we aren't applying the Symbol transform (which changes all typeof checks) so we'd have to do something similar with the type (already have that in place for RegExps and old browsers) - not hard but just a little bit more work.

Thoughts @sebmarkbage @spicyj

@sebmarkbage
Copy link
Collaborator

Yea. Seems reasonable. The .constructor === Symbol as a fallback is ok. Not ideal since it doesn't work cross-realm but we should use whatever is standard practice among popular polyfills.

@puradox
Copy link
Contributor

puradox commented Mar 29, 2016

Any possibility that this will be added in future versions?

@zpao
Copy link
Member

zpao commented Mar 29, 2016

Yes, we just never got back to it. If you're interested in working on it, this is something that would make a nice addition in 15.1.

@RaitoBezarius
Copy link
Contributor

@zpao No one is assigned? I could do a PR for it!

@gaearon
Copy link
Collaborator

gaearon commented Mar 31, 2016

GitHub doesn’t make it very obvious 😢

Usually when people start working on a feature they reference it, and it appears in the issue.
You can see the existing PR above:

screen shot 2016-03-31 at 14 20 27

@zpao
Copy link
Member

zpao commented Mar 31, 2016

Also, you can't assign issues to people who don't have commit access, making it much harder to communicate that somebody outside the core team is working on it.

@RaitoBezarius
Copy link
Contributor

That's a shame, it seems related to isaacs/github#369 and isaacs/github#100 :/

@ffxsam
Copy link

ffxsam commented Apr 14, 2016

Any update on this? I suppose I'll switch to string constants in this one instance, but symbols would've been better!

@RaitoBezarius
Copy link
Contributor

@ffxsam A PR is running on #6377 -- I don't know what is the status, but it looks complete on our side. Maybe, it will be merged soon!

@gaearon
Copy link
Collaborator

gaearon commented Apr 14, 2016

We’re still ironing out how we’ll move on with cutting branches on 15.x but once it’s decided I expect this to get in.

@sophiebits
Copy link
Collaborator

Looks like #6377 was merged. Should be in 15.1 or 15.2.

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

9 participants