Symbol Shim #74

Closed
wants to merge 11 commits into
from

Projects

None yet

3 participants

@kitsonk
Member
kitsonk commented Oct 16, 2015

This PR adds a Symbol shim which is compatible with ES6/ES2015 and offloads to the native when possible.

The shim does not augment the built-in Symbol if present, which, depending upon the browser, is missing several built-in common symbols that are part of the ES6 spec. It might be worth considering providing those symbols.

kitsonk added some commits Oct 16, 2015
@kitsonk kitsonk Add Symbol shim c2016a2
@kitsonk kitsonk Add Symbol and unit tests
02cce16
@kitsonk kitsonk Incorrectly cased module name 42ccccd
@kitsonk kitsonk Selinium version is problematic on BrowserStack
944447e
@kfranqueiro kfranqueiro and 1 other commented on an outdated diff Oct 16, 2015
tests/intern.ts
@@ -29,7 +29,7 @@ export const proxyUrl = 'http://localhost:9001/';
// Note that the `build` capability will be filled in with the current commit ID from the Travis CI environment
// automatically
export const capabilities = {
- 'browserstack.selenium_version': '2.46.0',
+ // 'browserstack.selenium_version': '2.46.0',
@kfranqueiro
kfranqueiro Oct 16, 2015 Member

Is this commented line really intended to be committed?

@kitsonk
kitsonk Oct 16, 2015 Member

It did while the CI/BrowserStack is being flakey... It is mainly a reminder that it needs to be addressed before it is landed formally.

@kfranqueiro kfranqueiro and 1 other commented on an outdated diff Oct 16, 2015
tsconfig.json
@@ -17,134 +17,137 @@
"./typings/tsd.d.ts"
],
"files": [
- "./src/DateObject.ts",
@kfranqueiro
kfranqueiro Oct 16, 2015 Member

I'm not sure why this entire array got regenerated/reordered in this commit. Was the usual grunt task still used to generate it?

(Sorry these comments are kind of nit-picks...)

@kitsonk
kitsonk Oct 16, 2015 Member

The latest builds of TypeScript/atom-typescript have regenerated the file without the relatives paths... we can keep ignoring it, or update it at some point. It wouldn't have to come in with this commit, but I just got tired of pretending the file didn't change.

@kfranqueiro
kfranqueiro Oct 16, 2015 Member

My point is that if I run grunt dev inside the repository, I get a bunch of changes right back to the order this file already was in. Shouldn't grunt dev typically be getting run before commits (without atom-typescript running to immediately clobber it again, I thought we had generally stopped using that)?

@kitsonk
kitsonk Oct 16, 2015 Member

That was after a grunt dev. Whatever the updateTsconfig task is doing in core isn't backing out those changes.

@kfranqueiro
Member

As someone still generally uninitiated to Symbols, can you explain what this would be used for? I had naively been under the impression that symbols were something that didn't necessarily make sense to shim.

@kitsonk
Member
kitsonk commented Oct 16, 2015

Having a shim for Symbol enables the possibility of some other items... like shimming Iterators. Trying to support the Iterator API becomes really silly without Symbols. Also, TypeScript can understand symbols, but will not polyfill the functionality, so in order to write TypeScript code that uses/supports Symbols and not have significant amounts of alternative code paths, a Symbol shim effectively does that, so a single code path. Again, Iterators are the main use case at the moment, but as more of ES6 comes to bear, many of the built ins and other objects will be created with Symbols.

On those browsers that support it (Safari 9+, Chrome, Firefox, NodeJS 4+, Opera, Edge), where Symbols are used, it reduces the overhead of processing text strings for property names (and is optimisation safe), while the non-supported browsers, it only introduces minimal overhead (and I still believe optimise safe).

@kitsonk kitsonk Reverse tsconfig.json
0873bb5
@kitsonk kitsonk added the discussion label Nov 17, 2015
kitsonk added some commits Dec 30, 2015
@kitsonk kitsonk Rebase. 413e13f
@kitsonk kitsonk Add Symbol and unit tests ccba78d
@kitsonk kitsonk Incorrectly cased module name 4b9fae1
@kitsonk kitsonk Merge branch 'symbol' of github.com:kitsonk/core into symbol
8d11f0d
@mwistrand mwistrand was assigned by morrinene Feb 24, 2016
@kitsonk kitsonk added this to the beta.1 milestone Feb 29, 2016
kitsonk added some commits Feb 29, 2016
@kitsonk kitsonk Merge branch 'master' into symbol e4b7e12
@kitsonk kitsonk Fix linting issue with Symbol
b390962
@kitsonk kitsonk referenced this pull request Feb 29, 2016
Closed

Shim module for Set #115

@mwistrand mwistrand commented on the diff Apr 1, 2016
src/Symbol.ts
+ configurable: configurable
+ };
+ }
+
+ const getSymbolName = (function () {
+ const created = create(null);
+ return function (desc: string|number): string {
+ let postfix = 0;
+ let name: string;
+ while (created[String(desc) + (postfix || '')]) {
+ ++postfix;
+ }
+ desc += String(postfix || '');
+ created[desc] = true;
+ name = '@@' + desc;
+ defineProperty(objPrototype, name, {
@mwistrand
mwistrand Apr 1, 2016 Contributor

I can understand why it would make sense to add the so-called "well-known" symbols to Object.prototype, but what reason is there for decorating Object.prototype with all shimmed symbols?

@kitsonk
kitsonk Apr 11, 2016 Member

What this does is ensure that there is a setter for the symbol that makes it non-enumerable... Otherwise you would run into this problem:

const obj = {};
obj[Symbol('foo')] = 'bar';
Object.keys(obj); // [ '@@foo' ]
/* instead of */
Object.keys(obj); // [ ]
@mwistrand
mwistrand Apr 11, 2016 Contributor

That makes sense; thanks for the explanation.

@mwistrand
Contributor

With regard to providing any missing well-known symbols, I see two paths forward:

  1. Decorate native Symbol with the missing well-knowns.
  2. Rework the module so that a wrapper is always used instead of either the shim or native Symbol directly.

Otherwise, how useful will the other shims be without the symbols they rely on? For example, as mentioned earlier, "trying to support the Iterator API becomes really silly without Symbols." If I want to write a custom iterator that will not need to be updated once the native implementations are in place, then I will expect to be able to use the format object[Symbol.iterator] = function () { ... };. So I do not see how we could get away with not including support for the missing symbols as the other shims are implemented.

@kitsonk kitsonk modified the milestone: 2016.04, beta.1 Apr 8, 2016
@kitsonk
Member
kitsonk commented Apr 11, 2016

I am going to address these comments in #115 since it seems a bit silly to update both branches at the moment. But for the Well Known Symbols, after thinking about it, I think it is just best to fill any missing well known symbols after we have resolved if we are using the native or shim. This in theory could create a well known symbol someone wasn't expecting, partially breaking the principal of don't interfere with other code but to hold to the principal would require a lot of work for little value.

@kitsonk
Member
kitsonk commented Apr 11, 2016

Closed via a202350

@kitsonk kitsonk closed this Apr 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment