Add new primitive PropType `Symbol` #6377

puradox · 2016-03-30T00:31:58Z

With the edition of ECMA-262, we now have a new primative type called Symbol. This primitive type should be added to the PropTypes, as users will eventually be using Symbols in their props for describing unique and immutable data, such as identifiers.

With the edition of ECMA-262, we now have a new primative type called Symbol. This primative type should be added to the PropTypes, as users will eventually be using Symbols in their props for describing unique and immutable data, such as identifiers.

I believe that I covered all the typical use cases for Symbols. If you think that I missed one, feel free to contribute.

puradox · 2016-03-30T00:34:05Z

This relates to issue #4917

gaearon · 2016-03-30T01:09:34Z

If the user is using a Babel polyfill, would a component defined with PropTypes.symbol print a warning?
How do we want to handle this scenario?

aweary · 2016-03-30T14:35:08Z

@gaearon I'd imagine that getPropType would return 'function' for a polyfill'd Symbol right? If so the warning could note that a PropTypes.symbol was found to be a function and warn that it could be either 1) an invalid prop or 2) due to a pollyfill.

borisyankov · 2016-03-30T18:17:07Z

I don't think showing a warning to users of polyfill is desirable.
Maybe, the code can 'interrogate' the function somehow and determine if it is a polyfill for Symbol?

puradox · 2016-03-30T23:20:40Z

@gaearon You bring up an interesting point. There could be a check if global._babelPolyfill == true (source) and then handle the polyfill of Symbol accordingly. From there, one possible solution is using instanceof instead of typeof.

brigand · 2016-03-31T09:54:50Z

We could do what babel compiles typeof x === 'symbol' to:

'use strict';

// refactored for readability
var nativeSymbolSupported = typeof Symbol === "function" && typeof Symbol.iterator === "symbol";
var _typeof = nativeSymbolSupported
 ? function (obj) { return typeof obj; }
 : function (obj) {
    if (obj && typeof Symbol === "function" && obj.constructor === Symbol) {
      return "symbol"
    }
    else {
      typeof obj; 
    }
  };

(typeof x === 'undefined' ? 'undefined' : _typeof(x)) === 'symbol';

puradox · 2016-03-31T19:58:01Z

@brigand I like that approach much better 👍

We could further simplify and extend this to getPropType() to return symbol make sure that functions aren't being mistaken for Symbols

// Equivalent of `typeof` but with special handling for array and regexp.
function getPropType(propValue) {
  var propType = typeof propValue;
  // ...
  if (propType === 'function' && propValue instanceof Symbol) {
    return 'symbol';
  }
  return propType;
}

RaitoBezarius · 2016-03-31T20:05:56Z

@puradox That's definitely a good idea! But how can you be sure that Symbol is available in the namespace? And if it is an instance of it? On my pull request, we can use sort of duck typing approach and say, if it behaves like a Symbol: all symbols have a toStringTag property which is equal to the default name (toString name) which is always: symbol.

What do you think so?

puradox · 2016-03-31T20:18:21Z

@RaitoBezarius You bring up a good point with Symbol being undefined in the global namespace. We can easily check for that using typeof Symbol

So the check in getPropType() would actually be

if (propType === 'function' && typeof Symbol === 'function' && propValue instanceof Symbol) {
  return 'symbol';
}

Does this address your concern?

RaitoBezarius · 2016-03-31T20:30:12Z

Awesome, according to core-js polyfill: https://github.com/zloirock/core-js/blob/master/modules/es6.symbol.js#L68 -- the test works well. (because core-js is always in the global namespace, I think?)

For this polyfill: https://github.com/medikoo/es6-symbol

→ https://github.com/medikoo/es6-symbol/blob/master/is-symbol.js

It sounds like that it's not that spec-compliant because it should be toStringTag and not @@toStringTag, but we could definitely check for this too.

For this polyfill: https://github.com/component/symbol
We won't detect it, seems like. It does not respect the spec, all we can know is: look for a __key__ property whose prototype property (Object.prototype.key) is equal to undefined (the getter returns nothing), but we can modify it using the setter and look for the toString method for changes. (The code is clearer than my text).

It should solve the problem of testing when no Symbol is available in the global namespace for a strange reason (polyfill is "sandboxed" or not leaked, etc...)

Would you like me to write some parts of the code and let you cherry-pick them on your branch so that we can complete this PR ?

puradox · 2016-03-31T20:38:42Z

@RaitoBezarius Yeah, it would be great if you could write some parts.

Most ES6 polyfills will add support by implementing `Symbol` as a function. This causes `typeof` to return `function` rather than `symbol` for polyfilled clients.

facebook-github-bot · 2016-03-31T20:52:00Z

@puradox updated the pull request.

1) If it is a native Symbol 2) If it is spec-compliant 3) Try to match non-spec compliant polyfill by a instanceof check on Symbol if it exists in the global namespace

…elements

Improve symbol check for (local) polyfills

RaitoBezarius · 2016-04-05T19:58:40Z

@gaearon What are you feelings on adding Symbols polyfills as devDependencies for the tests? Do you think it's a acceptable idea?

@puradox Will push some fix for the lint issues (my bad, some habits sticks when switching between projects :D), as for the test which fails, I'll merge back the current master of React inside my fork and try with this, will open two PR (one for the lint, one for the unit test fix).

Please, if you have some time, review the code so we can move forward and get this merged 🎉 !
Thanks a lot!

…s and trailing comma

Fix code-style issue with React guidelines

gaearon · 2016-04-06T11:47:37Z

We will review this after 15.0 is out. Feel free to ping me if we forget to (but we shouldn't).

puradox · 2016-04-07T23:04:41Z

Can we get someone with write access to rerun the Travis-CI tests?

Cloning into 'facebook/react'...
remote: GitHub is offline for maintenance. See http://status.github.com for more info.
fatal: unable to access 'https://github.com/facebook/react.git/': The requested URL returned error: 503

jimfb · 2016-04-07T23:05:49Z

Rerunning now

There seems to be a bug with medikoo/es6-symbol where the global state affects the implementation of the polyfill. I found this by running the individual test file alone then running all the tests using `grunt test`. I found that it passes when ran alone and failed when ran collectively. I did not find this buggy behavior with zloirock/core-js's implementation of a polyfill for Symbol. Thus, removing I will keep the more popular polyfill (core-js) and remove the buggy polyfill (es6-symbol). If you are reading this and can think of a confounding factor that is causing this bug, please let me know and we can try to work together to add support for medikoo/es6-symbol.

facebook-github-bot · 2016-04-08T01:01:51Z

@puradox updated the pull request.

gaearon · 2016-04-08T22:24:42Z

package.json

    "coveralls": "^2.11.6",
    "del": "^2.0.2",
    "derequire": "^2.0.3",
+    "es6-symbol": "^3.0.2",


Is this necessary? Doesn’t appear to be used.

It's a leftover from a @puradox fix on tests — I will clean up this!

On Sat, Apr 9, 2016, 00:26 Dan Abramov notifications@github.com wrote:

In package.json
#6377 (comment):

"coveralls": "^2.11.6", "del": "^2.0.2", "derequire": "^2.0.3",

"es6-symbol": "^3.0.2",

Is this necessary? Doesn’t appear to be used.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/facebook/react/pull/6377/files/ad94295f139a8190286852e14a5c731065f5f0ba#r59097655

Ryan Lahfa, de mon téléphone.

Thank you @RaitoBezarius! I forgot to remove this dependency when removing unit test support for es6-symbol.

I have pushed a fix on your fork to remove the dependency, so that we can
unblock the code review.

Kind regards, Ryan Lahfa

Remove unused ES6 Symbol dependency

facebook-github-bot · 2016-04-11T21:42:27Z

@puradox updated the pull request.

gaearon · 2016-05-10T14:09:17Z

Merging since this was accepted.

Add new primitive PropType `Symbol` (cherry picked from commit 7bf96c0)

React has introduced a new primitive PropType "symbol". facebook/react#6377 This change adds support for that propType so that it is not reported as "custom".

puradox added 2 commits March 29, 2016 17:18

Added Symbol primative PropType.

97bcce5

With the edition of ECMA-262, we now have a new primative type called Symbol. This primative type should be added to the PropTypes, as users will eventually be using Symbols in their props for describing unique and immutable data, such as identifiers.

Added tests for the new primative, Symbol.

75c4e03

I believe that I covered all the typical use cases for Symbols. If you think that I missed one, feel free to contribute.

facebook-github-bot added GH Review: review-needed CLA Signed labels Mar 30, 2016

gaearon mentioned this pull request Mar 31, 2016

Add Symbol prop type #6387

Closed

Support for ES6 polyfills

5485ac8

Most ES6 polyfills will add support by implementing `Symbol` as a function. This causes `typeof` to return `function` rather than `symbol` for polyfilled clients.

RaitoBezarius added 4 commits April 2, 2016 10:10

prop-types(symbols): add a helper is-symbol function

13d9720

devDependencies: add core-js and es6-symbol polyfill for tests

0d8ab0e

prop-types(symbols): rewrite isSymbol to be simpler

4239f8a

1) If it is a native Symbol 2) If it is spec-compliant 3) Try to match non-spec compliant polyfill by a instanceof check on Symbol if it exists in the global namespace

prop-types(symbols,tests): test for polyfilled Symbol and non-Symbol …

c291f8a

…elements

gaearon mentioned this pull request Apr 4, 2016

Dead code eliminate React.PropTypes checkers in production #6401

Closed

Merge pull request #1 from RaitoBezarius/fork/proptypes-symbol

f846b54

Improve symbol check for (local) polyfills

RaitoBezarius and others added 2 commits April 5, 2016 22:01

prop-types(tests,code-style): fix missing semicolon, unexpected space…

a98e2cb

…s and trailing comma

Merge pull request #2 from RaitoBezarius/fork/proptypes-symbol

ced7a3d

Fix code-style issue with React guidelines

gaearon reviewed Apr 8, 2016
View reviewed changes

RaitoBezarius and others added 2 commits April 11, 2016 20:19

package(dependency): remove unused es6-symbol dependency

95fed01

Merge pull request #3 from RaitoBezarius/fork/proptypes-symbol

6e4f3ea

Remove unused ES6 Symbol dependency

gaearon added this to the 15.x milestone Apr 14, 2016

RaitoBezarius mentioned this pull request Apr 14, 2016

Support es6 symbols in proptype #4917

Closed

jimfb added GH Review: accepted and removed GH Review: review-needed labels Apr 14, 2016

gaearon merged commit 7bf96c0 into facebook:master May 10, 2016

zpao added the semver-minor label May 16, 2016

zpao modified the milestones: 15.y.0, 15-next Jun 1, 2016

zpao pushed a commit to zpao/react that referenced this pull request Jun 8, 2016

Merge pull request facebook#6377 from puradox/proptypes-symbol

d2635f2

Add new primitive PropType `Symbol` (cherry picked from commit 7bf96c0)

zpao pushed a commit that referenced this pull request Jun 14, 2016

Merge pull request #6377 from puradox/proptypes-symbol

f112083

Add new primitive PropType `Symbol` (cherry picked from commit 7bf96c0)

zpao modified the milestones: 15-next, 15.2.0 Jun 14, 2016

zpao mentioned this pull request Aug 29, 2016

Support React.ProptTypes.symbol facebook/flow#2362

Closed

ZauberNerd mentioned this pull request Sep 5, 2016

Add support for newly introduced PropTypes "symbol" reactjs/react-docgen#123

Merged

renovate bot mentioned this pull request Feb 2, 2018

Update dependency react to v0.14.9 - autoclosed signavio/backbone-rel#29

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new primitive PropType `Symbol` #6377

Add new primitive PropType `Symbol` #6377

puradox commented Mar 30, 2016

puradox commented Mar 30, 2016

gaearon commented Mar 30, 2016

aweary commented Mar 30, 2016

borisyankov commented Mar 30, 2016

puradox commented Mar 30, 2016

brigand commented Mar 31, 2016

puradox commented Mar 31, 2016

RaitoBezarius commented Mar 31, 2016

puradox commented Mar 31, 2016

RaitoBezarius commented Mar 31, 2016

puradox commented Mar 31, 2016

facebook-github-bot commented Mar 31, 2016

RaitoBezarius commented Apr 5, 2016

gaearon commented Apr 6, 2016

puradox commented Apr 7, 2016

jimfb commented Apr 7, 2016

facebook-github-bot commented Apr 8, 2016

gaearon Apr 8, 2016

RaitoBezarius Apr 8, 2016

puradox Apr 8, 2016

RaitoBezarius Apr 11, 2016

facebook-github-bot commented Apr 11, 2016

gaearon commented May 10, 2016

Add new primitive PropType Symbol #6377

Add new primitive PropType Symbol #6377

Conversation

puradox commented Mar 30, 2016

puradox commented Mar 30, 2016

gaearon commented Mar 30, 2016

aweary commented Mar 30, 2016

borisyankov commented Mar 30, 2016

puradox commented Mar 30, 2016

brigand commented Mar 31, 2016

puradox commented Mar 31, 2016

RaitoBezarius commented Mar 31, 2016

puradox commented Mar 31, 2016

RaitoBezarius commented Mar 31, 2016

puradox commented Mar 31, 2016

facebook-github-bot commented Mar 31, 2016

RaitoBezarius commented Apr 5, 2016

gaearon commented Apr 6, 2016

puradox commented Apr 7, 2016

jimfb commented Apr 7, 2016

facebook-github-bot commented Apr 8, 2016

gaearon Apr 8, 2016

Choose a reason for hiding this comment

RaitoBezarius Apr 8, 2016

Choose a reason for hiding this comment

puradox Apr 8, 2016

Choose a reason for hiding this comment

RaitoBezarius Apr 11, 2016

Choose a reason for hiding this comment

facebook-github-bot commented Apr 11, 2016

gaearon commented May 10, 2016

Add new primitive PropType `Symbol` #6377

Add new primitive PropType `Symbol` #6377