Skip to content

refactor: minor code cleanup / fixes #3

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 8 commits into from
Closed

refactor: minor code cleanup / fixes #3

wants to merge 8 commits into from

Conversation

vicb
Copy link

@vicb vicb commented Feb 10, 2014

@mhevery this PR cleanups your branch and fixes some minor items. I'll use this as a starting point for my coming work on change detection.

mhevery and others added 7 commits February 5, 2014 16:12
@mhevery
chore(input): Add missing test for clearing input type number
0638068 
Closes dart-archive#415
@bgourlie @mhevery
feat(sanitization): make NodeValidator injectable
42a8c9a 
Closes dart-archive#490
Closes dart-archive#498
@chirayuk @mhevery
fix(ng-class): array syntax should not insert nulls
0bb3189 
Closes dart-archive#513
@matsko @mhevery
fix(ngModel): evaluate user input using onInput instead of onKeyDown
49f3fab
@mhevery
chore(perf): Get watch_group_perf running on Dart2js
3007882 
DARTIUM:
========
fieldRead: => 58,240 ops/sec (17 us) stdev(0.0028)
fieldReadGetter: => 1,480,972 ops/sec (1 us) stdev(0.08649)
mapRead: => 1,720,752 ops/sec (1 us) stdev(0.05972)
obj.method?(): => 93,362 ops/sec (11 us) stdev(0.00549)
obj.method?(obj): => 79,881 ops/sec (13 us) stdev(0.00301)
add?(a, a): => 1,342,560 ops/sec (1 us) stdev(0.10907)

change-detect List[1000](RunTime): 61.251990689697415 us.

DART2JS:
========
fieldRead: => 372,422 ops/sec (3 us) stdev(0.0109)
fieldReadGetter: => 1,038,660 ops/sec (1 us) stdev(0.19283)
mapRead: => 721,103 ops/sec (1 us) stdev(0.04434)
obj.method?(): => 172,910 ops/sec (6 us) stdev(0.00801)
obj.method?(obj): => 161,329 ops/sec (6 us) stdev(0.00598)
add?(a, a): => 2,432,982 ops/sec (0 us) stdev(0.17624)

change-detect List[1000](RunTime): 145.12734924896597 us.
@mhevery
feat(scope2): Basic implementation of Scope v2
c0630d9
@mhevery
feat(Scope): Brand new scope implementation which takes advantage of …
ea1c175 
…the new change detection
@vicb
Copy link
Author

vicb commented Feb 10, 2014

There are some issues with this PR, I'm fixing this.

@vicb
Copy link
Author

vicb commented Feb 10, 2014

Failures are already there in your branch. I'll try to tackle them as part of the incoming PRs - this one could be merged.

@vicb vicb mentioned this pull request Feb 10, 2014
Closed
mhevery
mhevery reviewed Feb 11, 2014
View reviewed changes
lib/change_detection/ast.dart
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the { on the new line in this case. It is hard to tell where the body starts otherwise. I know it is not consistent with the rest of the style, but to me it is more readable. What do you think?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I think this is easier to read

foo 
  ? true
  : false

then

foo ?
  true :
  false

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really care for the {, new line might indeed be more readable, will change.

about the ternary op, the core Dart team seem to prefer ops at the end of line but I also found ops at strart of line more readable, will change also.

@mhevery
Copy link
Owner

mhevery commented Feb 11, 2014

MERGED

1 similar comment
@mhevery
Copy link
Owner

mhevery commented Feb 11, 2014

MERGED

@vicb
Copy link
Author

vicb commented Feb 12, 2014

closing as merged

@vicb vicb closed this Feb 12, 2014
mhevery pushed a commit that referenced this pull request May 15, 2014
@chirayuk
fix(Dirty Checking): fix watching methods/closures
d71c7fa 
-  BUG: DynamicFieldGetterFactory::isMethod did not handle methods
   defined in superclasses.
-  BUG: Upon detecting a method, the code assumed that you would only
   invoke it.  This broke application code that watched a method (e.g.
   by way of Component mapping) just to get the closurized value and
   store it somewhere to invoke later (test case and stack trace at end
   of this commit message.)
-  BUG: StaticFieldGetterFactory::method() and
   DynamicFieldGetterFactory::method() differed.  There was no
   difference between StaticFieldGetterFactory::method() and
   StaticFieldGetterFactory::getter(). 
   DynamicFieldGetterFactory::method(), as mentioned before, assumed
   that the only thing you could do with it was to invoke it (i.e. not a
   leaf watch.)
-  There was very little testing for StaticFieldGetterFactory.  This
   meant that, though it was out of sync with DynamicFieldGetterFactory,
   no tests were failing.

Changes in this commit:

- run the same tests against StaticFieldGetterFactory that are run
  against DynamicFieldGetterFactory
- do not call the result of GetterFactory.method()
  in "set object(value)"
- reduce the difference between the two different factories. 
  GetterFactory now only has one method in it's interface definition -
  the getter function.  `_MODE_METHOD_INVOKE_`, `isMethod`,
  `isMethodInvoke`, etc. are gone.

**Bug Details:**

Refer to the repro case at
chirayuk/angular.dart@issue_999^...issue_999

```dart
// Given this object.
class Foo {
  bar(x) => x+1;
}

// This test case (in an appropriate file like `scope_spec.dart`) fails
// with a traceback

it('should watch closures', (RootScope rootScope, Logger log) {
  rootScope.context['foo'] = new Foo();
  rootScope.context['func'] = null;
  rootScope.watch('foo.bar', (v, _) { rootScope.context['func'] = v; });
  rootScope.watch('func(1)', (v, o) => log([v, o]));
  rootScope.apply();
  expect(log).toEqual([[null, null], [2, null]]);
});
```

**Stack Trace:**

    Chrome 34.0.1847 (Mac OS X 10.9.2) scope watch/digest should watch closures FAILED
    Test failed: Caught Closure call with mismatched arguments: function 'DynamicFieldGetterFactory.method.<anonymous closure>'

    NoSuchMethodError: incorrect number of arguments passed to method named 'DynamicFieldGetterFactory.method.<anonymous closure>'
    Receiver: Closure: (List, Map) => dynamic
    Tried calling: DynamicFieldGetterFactory.method.<anonymous closure>(Instance of 'Foo')
    Found: DynamicFieldGetterFactory.method.<anonymous closure>(args, namedArgs)
    #0      Object.noSuchMethod (dart:core-patch/object_patch.dart:45)
    #1      DirtyCheckingRecord.object= (package:angular/change_detection/dirty_checking_change_detector.dart:465:78)
    #2      _FieldHandler.acceptValue (package:angular/change_detection/watch_group.dart:630:17)
    #3      WatchGroup.addFieldWatch (package:angular/change_detection/watch_group.dart:171:29)
    #4      FieldReadAST.setupWatch (package:angular/change_detection/ast.dart:67:31)
    #5      WatchGroup.watch.<anonymous closure> (package:angular/change_detection/watch_group.dart:144:40)
    #6      _HashMap.putIfAbsent (dart:collection-patch/collection_patch.dart:124)
    #7      WatchGroup.watch (package:angular/change_detection/watch_group.dart:143:27)
    #8      Scope.watch (package:angular/core/scope.dart:240:31)
    #9      main.<anonymous closure>.<anonymous closure>.<anonymous closure> (/Users/chirayu/work/angular.dart/test/core/scope_spec.dart:1308:24)
    #10     _LocalInstanceMirror._invoke (dart:mirrors-patch/mirrors_impl.dart:440)
    #11     _LocalInstanceMirror.invoke (dart:mirrors-patch/mirrors_impl.dart:436)
    #12     _LocalClosureMirror.apply (dart:mirrors-patch/mirrors_impl.dart:466)
    #13     DynamicInjector.invoke (package:di/dynamic_injector.dart:97:20)
    #14     _SpecInjector.inject (package:angular/mock/test_injection.dart:58:22)
    #15     inject.<anonymous closure> (package:angular/mock/test_injection.dart:100:44)
    #16     _rootRun (dart:async/zone.dart:723)
    #17     _ZoneDelegate.run (dart:async/zone.dart:453)
    #18     _CustomizedZone.run (dart:async/zone.dart:663)
    dart-archive#19     runZoned (dart:async/zone.dart:954)
    dart-archive#20     _syncOuter.<anonymous closure> (package:angular/mock/zone.dart:227:22)
    dart-archive#21     _withSetup.<anonymous closure> (/Users/chirayu/work/angular.dart/test/jasmine_syntax.dart:13:14)
    dart-archive#22     _run.<anonymous closure> (package:unittest/src/test_case.dart:102:27)
    dart-archive#23     _rootRunUnary (dart:async/zone.dart:730)
    dart-archive#24     _ZoneDelegate.runUnary (dart:async/zone.dart:462)
    dart-archive#25     _CustomizedZone.runUnary (dart:async/zone.dart:667)
    dart-archive#26     _Future._propagateToListeners.handleValueCallback (dart:async/future_impl.dart:488)
    dart-archive#27     _Future._propagateToListeners (dart:async/future_impl.dart:571)
    dart-archive#28     _Future._completeWithValue (dart:async/future_impl.dart:331)
    dart-archive#29     _Future._asyncComplete.<anonymous closure> (dart:async/future_impl.dart:393)
    dart-archive#30     _rootRun (dart:async/zone.dart:723)
    dart-archive#31     _ZoneDelegate.run (dart:async/zone.dart:453)
    dart-archive#32     _CustomizedZone.run (dart:async/zone.dart:663)
    dart-archive#33     _BaseZone.runGuarded (dart:async/zone.dart:574)
    dart-archive#34     _BaseZone.bindCallback.<anonymous closure> (dart:async/zone.dart:599)
    dart-archive#35     _asyncRunCallbackLoop (dart:async/schedule_microtask.dart:23)
    dart-archive#36     _asyncRunCallback (dart:async/schedule_microtask.dart:32)
    dart-archive#37     _handleMutation (file:///Volumes/data/b/build/slave/dartium-mac-full-dev/build/src/dart/tools/dom/src/native_DOMImplementation.dart:588)

    DECLARED AT:#0      inject (package:angular/mock/test_injection.dart:97:5)
    #1      _injectify (/Users/chirayu/work/angular.dart/test/_specs.dart:236:25)
    #2      iit (/Users/chirayu/work/angular.dart/test/_specs.dart:244:53)
    #3      main.<anonymous closure>.<anonymous closure> (/Users/chirayu/work/angular.dart/test/core/scope_spec.dart:1305:10)
    #4      describe.<anonymous closure> (/Users/chirayu/work/angular.dart/test/jasmine_syntax.dart:62:9)
    #5      group (package:unittest/unittest.dart:396:9)
    #6      describe (/Users/chirayu/work/angular.dart/test/jasmine_syntax.dart:60:15)
    #7      describe (/Users/chirayu/work/angular.dart/test/_specs.dart:248:46)
    #8      main.<anonymous closure> (/Users/chirayu/work/angular.dart/test/core/scope_spec.dart:1009:13)
    #9      describe.<anonymous closure> (/Users/chirayu/work/angular.dart/test/jasmine_syntax.dart:62:9)
    #10     group (package:unittest/unittest.dart:396:9)
    #11     describe (/Users/chirayu/work/angular.dart/test/jasmine_syntax.dart:60:15)
    #12     describe (/Users/chirayu/work/angular.dart/test/_specs.dart:248:46)
    #13     main (/Users/chirayu/work/angular.dart/test/core/scope_spec.dart:14:11)
    #14     main.<anonymous closure> (http://localhost:8765/__adapter_dart_unittest.dart:169:15)
    #15     _rootRun (dart:async/zone.dart:723)
    #16     _ZoneDelegate.run (dart:async/zone.dart:453)
    #17     _CustomizedZone.run (dart:async/zone.dart:663)
    #18     runZoned (dart:async/zone.dart:954)
    dart-archive#19     main (http://localhost:8765/__adapter_dart_unittest.dart:146:11)

    #0      _SpecInjector.inject (package:angular/mock/test_injection.dart:60:7)
    #1      inject.<anonymous closure> (package:angular/mock/test_injection.dart:100:44)
    #2      _rootRun (dart:async/zone.dart:723)
    #3      _rootRun (dart:async/zone.dart:724)
    #4      _rootRun (dart:async/zone.dart:724)
    #5      _ZoneDelegate.run (dart:async/zone.dart:453)
    #6      _CustomizedZone.run (dart:async/zone.dart:663)
    #7      runZoned (dart:async/zone.dart:954)
    #8      _syncOuter.<anonymous closure> (package:angular/mock/zone.dart:227:22)
    #9      _withSetup.<anonymous closure> (/Users/chirayu/work/angular.dart/test/jasmine_syntax.dart:13:14)
    #10     _withSetup.<anonymous closure> (/Users/chirayu/work/angular.dart/test/jasmine_syntax.dart:14:5)
    #11     _withSetup.<anonymous closure> (/Users/chirayu/work/angular.dart/test/jasmine_syntax.dart:14:5)
    #12     _run.<anonymous closure> (package:unittest/src/test_case.dart:102:27)
    #13     _rootRunUnary (dart:async/zone.dart:730)
    #14     _ZoneDelegate.runUnary (dart:async/zone.dart:462)
    #15     _CustomizedZone.runUnary (dart:async/zone.dart:667)
    #16     _Future._propagateToListeners.handleValueCallback (dart:async/future_impl.dart:488)
    #17     _Future._propagateToListeners (dart:async/future_impl.dart:571)
    #18     _Future._completeWithValue (dart:async/future_impl.dart:331)
    dart-archive#19     _Future._asyncComplete.<anonymous closure> (dart:async/future_impl.dart:393)
    dart-archive#20     _rootRun (dart:async/zone.dart:723)
    dart-archive#21     _ZoneDelegate.run (dart:async/zone.dart:453)
    dart-archive#22     _CustomizedZone.run (dart:async/zone.dart:663)
    dart-archive#23     _BaseZone.runGuarded (dart:async/zone.dart:574)
    dart-archive#24     _BaseZone.bindCallback.<anonymous closure> (dart:async/zone.dart:599)
    dart-archive#25     _asyncRunCallbackLoop (dart:async/schedule_microtask.dart:23)
    dart-archive#26     _asyncRunCallback (dart:async/schedule_microtask.dart:32)
    dart-archive#27     _handleMutation (file:///Volumes/data/b/build/slave/dartium-mac-full-dev/build/src/dart/tools/dom/src/native_DOMImplementation.dart:588)

Closes dart-archive#999
mhevery pushed a commit that referenced this pull request Jul 1, 2014
@chirayuk
fix(ng-view): cleanup should not destroy an already destroyed scope
5cb46a1 
Refer dart-archive#1182
and repro https://github.com/chirayuk/sample/tree/issue_1182_leaving_a_nested_ng_view

NgView's register cleanup handlers this way:

    _leaveSubscription = route.onLeave.listen((_) {
      _leaveSubscription.cancel();
      // …
      _cleanUp();
    });

When there are nested ng-views, upon a route change, the parent NgView
calls it's _cleanUp() first (which destroys it's child scope) and then
the child NgView attempts a cleanup.  However, it's child scope is
already detached due to the parent NgView cleaning up causing an
exception.

Stack trace is:

    'package:angular/core/scope.dart': Failed assertion: line 335 pos 12: 'isAttached' is not true.

    STACKTRACE:
    #0      Scope.destroy (package:angular/core/scope.dart:335:12)
    #1      NgView._cleanUp (package:angular/routing/ng_view.dart:130:24)
    #2      NgView._show.<anonymous closure> (package:angular/routing/ng_view.dart:106:15)
    #3      _rootRunUnary (dart:async/zone.dart:730)
    #4      _ZoneDelegate.runUnary (dart:async/zone.dart:462)
    #5      _onRunUnary.<anonymous closure> (package:angular/core/zone.dart:113:63)
    #6      VmTurnZone._onRunBase (package:angular/core/zone.dart:97:16)
    #7      _onRunUnary (package:angular/core/zone.dart:113:17)
    #8      _ZoneDelegate.runUnary (dart:async/zone.dart:462)
    #9      _CustomizedZone.runUnary (dart:async/zone.dart:667)
    #10     _BaseZone.runUnaryGuarded (dart:async/zone.dart:582)
    #11     _BufferingStreamSubscription._sendData (dart:async/stream_impl.dart:333)
    #12     _BufferingStreamSubscription._add (dart:async/stream_impl.dart:263)
    #13     _SyncBroadcastStreamController._sendData.<anonymous closure> (dart:async/broadcast_stream_controller.dart:344)
    #14     _BroadcastStreamController._forEachListener (dart:async/broadcast_stream_controller.dart:297)
    #15     _SyncBroadcastStreamController._sendData (dart:async/broadcast_stream_controller.dart:343)
    #16     _BroadcastStreamController.add (dart:async/broadcast_stream_controller.dart:227)
    #17     Router._leaveCurrentRouteHelper (package:route_hierarchical/client.dart:654:48)
    #18     Router._leaveCurrentRouteHelper (package:route_hierarchical/client.dart:656:47)
    dart-archive#19     Router._leaveCurrentRoute (package:route_hierarchical/client.dart:645:41)
    dart-archive#20     Router._leaveOldRoutes (package:route_hierarchical/client.dart:525:30)
    dart-archive#21     Router._processNewRoute (package:route_hierarchical/client.dart:497:27)
    dart-archive#22     Router._route.<anonymous closure> (package:route_hierarchical/client.dart:481:29)
    dart-archive#23     _rootRunUnary (dart:async/zone.dart:730)
    dart-archive#24     _ZoneDelegate.runUnary (dart:async/zone.dart:462)
    dart-archive#25     _onRunUnary.<anonymous closure> (package:angular/core/zone.dart:113:63)
    dart-archive#26     VmTurnZone._onRunBase (package:angular/core/zone.dart:97:16)
    dart-archive#27     _onRunUnary (package:angular/core/zone.dart:113:17)
    dart-archive#28     _ZoneDelegate.runUnary (dart:async/zone.dart:462)
    dart-archive#29     _CustomizedZone.runUnary (dart:async/zone.dart:667)
    dart-archive#30     _Future._propagateToListeners.handleValueCallback (dart:async/future_impl.dart:488)
    dart-archive#31     _Future._propagateToListeners (dart:async/future_impl.dart:571)
    dart-archive#32     _Future._completeWithValue (dart:async/future_impl.dart:331)
    dart-archive#33     _Future._asyncComplete.<anonymous closure> (dart:async/future_impl.dart:393)
    dart-archive#34     _rootRun (dart:async/zone.dart:723)
    dart-archive#35     _ZoneDelegate.run (dart:async/zone.dart:453)
    dart-archive#36     _onScheduleMicrotask.<anonymous closure> (package:angular/core/zone.dart:117:43)
    dart-archive#37     VmTurnZone._finishTurn (package:angular/core/zone.dart:143:34)
    dart-archive#38     VmTurnZone._onRunBase (package:angular/core/zone.dart:104:43)
    dart-archive#39     _onRunUnary (package:angular/core/zone.dart:113:17)
    dart-archive#40     _ZoneDelegate.runUnary (dart:async/zone.dart:462)
    dart-archive#41     _CustomizedZone.runUnary (dart:async/zone.dart:667)
    dart-archive#42     _BaseZone.runUnaryGuarded (dart:async/zone.dart:582)
    dart-archive#43     _BaseZone.bindUnaryCallback.<anonymous closure> (dart:async/zone.dart:608)

Closes dart-archive#1182
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vicb @mhevery @bgourlie @chirayuk @matsko