Shim module for Set #115

Closed
wants to merge 22 commits into
from

Projects

None yet

4 participants

@kitsonk
Member
kitsonk commented Feb 29, 2016

This module requires #74 and shims the functionality of Set, which offloads to native (when there is a compliant es6 Set implementation, IE 11 and older versions of Safari do not provide sufficient functionality, and so the shim is used in those situations).

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
@kitsonk kitsonk Reverse tsconfig.json
0873bb5
@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
@kitsonk kitsonk Merge branch 'master' into symbol e4b7e12
@kitsonk kitsonk Fix linting issue with Symbol
b390962
@kitsonk kitsonk Add iterables and WIP Set ea24522
@kitsonk kitsonk Add UTF-16 string iterable test 52f7081
@kitsonk kitsonk Complete Set and add unit tests
96a56d2
@kitsonk kitsonk added this to the beta.1 milestone Feb 29, 2016
@mwistrand mwistrand was assigned by kitsonk Feb 29, 2016
@kitsonk
Member
kitsonk commented Feb 29, 2016

I should also mention that it includes a module named iterator which provides a shim for common iterator type functionality, including the ability to replicate the for ... of type of iteration via a forOf() function. Where you can take something that defines a [Symbol.iterator] or iterators over anything that is ArrayLike, for example:

import { forOf } from 'dojo-core/iterator`;

const arr: number[] = [ 1, 2, 3 ];
forOf(arr, function (value, source, doBreak) {
  console.log(value);
  /* source is the original iterator object */
  /* doBreak() is a function which allows you to break the forOf loop */
});

Since it isn't possible to "shim" syntax, it isn't possible to offload to native fully, but it does grab whatever iterator might be available and iterates over that until done.

@kitsonk kitsonk Changes so dist/typings can be consumed
687e5d7
@kitsonk
Member
kitsonk commented Mar 1, 2016

So, it appears that TypeScript treats Symbol as a special object and elides it from typing information in a declaration. So even with an explicit import it still got elided. It would then cause issues in that the resulting .d.ts in the distribution would have issues with Symbol not being found. The only work around I was able to get to work was to provide a global Symbol .d.ts file which consumers will need to include in the scope of the project (or include lib.es6.d.ts or any other polyfil that declares Symbol in the global scope). At that point, the typings will be correct and the run-time behaviour will be preserved.

kitsonk added some commits Mar 7, 2016
@kitsonk kitsonk Merge remote-tracking branch 'upstream/master' into set-shim 1f0b7e8
@kitsonk kitsonk Merge branch 'master' into set-shim
19cc87d
@kitsonk kitsonk Merge branch 'master' into set-shim
5469b2e
@kitsonk kitsonk Merge branch 'master' into set-shim
57a31f4
@codecov-io

Current coverage is 97.96%

Merging #115 into master will not affect coverage as of 37fead9

@@            master    #115   diff @@
======================================
  Files           50      50       
  Stmts         2700    2700       
  Branches       518     518       
  Methods        609     609       
======================================
  Hit           2645    2645       
  Partial          2       2       
  Missed          53      53       

Review entire Coverage Diff as of 37fead9

Powered by Codecov. Updated on successful CI builds.

@kitsonk kitsonk referenced this pull request in dojo/meta Mar 18, 2016
Closed

Distribution Formats #18

@kitsonk kitsonk Merge branch 'master' into set-shim
ff3b48b
@mwistrand mwistrand commented on an outdated diff Apr 3, 2016
@@ -0,0 +1,118 @@
+import global from './global';
+import Symbol from './Symbol';
+import { IterableIterator, Iterable, ShimIterator } from './iterator';
+import { hasClass } from './decorators';
+import { forOf } from './iterator';
@mwistrand
mwistrand Apr 3, 2016 Contributor

This currently imports from ./iterator twice.

@mwistrand mwistrand and 1 other commented on an outdated diff Apr 3, 2016
+ clear(): void {
+ this._setData.length = 0;
+ };
+
+ delete(value: T): boolean {
+ const idx = this._setData.indexOf(value);
+ if (idx === -1) {
+ return false;
+ }
+ this._setData.splice(idx, 1);
+ return true;
+ };
+
+ entries(): IterableIterator<[T, T]> {
+ const ent = new ShimIterator(this._setData.map((value) => [ value, value ]));
+ (<any> ent)[Symbol.iterator] = function (): IterableIterator<[T, T]> {
@mwistrand
mwistrand Apr 3, 2016 Contributor

As far as I can tell, none of Dojo 2's target environments support just Symbol without Symbol.iterator, so this might work in all target environments without a hitch.

@kitsonk
kitsonk Apr 11, 2016 Member

Ok, I think I figured out what your point was. When I do a next push, it will hopefully address this point.

@mwistrand mwistrand and 1 other commented on an outdated diff Apr 3, 2016
+ (<any> values)[Symbol.iterator] = function (): IterableIterator<T> {
+ return <any> values;
+ };
+ return <any> values;
+ };
+
+ [Symbol.iterator](): IterableIterator<T> {
+ const iterator = new ShimIterator(this._setData);
+ (<any> iterator)[Symbol.iterator] = function (): IterableIterator<T> {
+ return <any> iterator;
+ };
+ return <any> iterator;
+ };
+
+ [Symbol.toStringTag]: string = 'Set';
+ }
@mwistrand
mwistrand Apr 3, 2016 Contributor

Since Set.prototype[@@iterator] is the same function as Set.prototype.values, and since the implementation is currently identical (apart from the iterator variable name), this duplicated method could be shortened.

@mwistrand
mwistrand Apr 5, 2016 Contributor

Also, [Symbol.toStringTag] will break in environments that support Symbol but not Symbol.toStringTag (e.g., the current version of Firefox (45.0.1)), as the dojo/Symbol currently does not add missing well-knowns.

@kitsonk
kitsonk Apr 8, 2016 Member

Good point, the shim for Symbol should do all the ES6 well knowns.

Also, will look at your other comment.

@mwistrand mwistrand and 1 other commented on an outdated diff Apr 3, 2016
src/iterator.ts
+ */
+export function forOf<T>(iterable: Iterable<T> | ArrayLike<T> | string, callback: ForOfCallback<T>, thisArg?: any): void {
+ let broken = false;
+
+ function doBreak() {
+ broken = true;
+ }
+
+ /* We need to handle iteration of double byte strings properly */
+ if (!isIterable(iterable) && typeof iterable === 'string') {
+ const l = iterable.length;
+ for (let i = 0; i < l; ++i) {
+ let char = iterable[i];
+ if ((i + 1) < l) {
+ const code = char.charCodeAt(0);
+ if ((code >= 0xD800) && (code <= 0xDBFF)) {
@mwistrand
mwistrand Apr 3, 2016 Contributor

These characters are currently exported by dojo/string as the constants HIGH_SURROGATE_MIN and HIGH_SURROGATE_MAX, respectively.

@kitsonk
kitsonk Apr 8, 2016 Member

Awesome, thanks!

@kitsonk kitsonk modified the milestone: 2016.04, beta.1 Apr 8, 2016
@kitsonk kitsonk referenced this pull request Apr 11, 2016
Closed

Symbol Shim #74

@kitsonk kitsonk Address PR feedback
cf821d4
@kitsonk
Member
kitsonk commented Apr 11, 2016

@mwistrand if you could take one more look, specifically at the changes I made (and any outstanding feedback you don't think I addressed) and then I think I can land this. I didn't directly update the Symbol branch (#74) as I just figured I would do it all in this PR.

@mwistrand
Contributor

Thanks, @kitsonk, for your responses and changes. Any concern/suggestion I had has been addressed, for both this and #74.

@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