Improve foreach performance by using arrayChange event #2324
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Normally when working with manipulating large arrays, I usually just reset the entire observable array to a new value and avoid individual pushes. So I don't believe I'm using destroy/destroyAll. |
|
Never used detroy/destroyAll , we are using ko heavily in a large large project with 150K+ lines of js. |
…mance when known operations make changes to large arrays. In addition, add ko.isObservableArray and subscription.disposeWhenNodeIsRemoved
This was referenced Dec 3, 2017
|
I use the |
|
I guess this optimisation won't work with "foreach: { data: expression }" syntax, as it will be neutralized by the re-template-everything-when-any-binding-definition-dependency-coughs undesired behavior of "template: { someThing: someExpression, ... }" syntax, as in #2332. edit: if data expression is a subscribable, it should be OK. But a binding à la "foreach: { data: array.sorted() }" won't make it. |
|
The syntax doesn't matter with |
brianmhunt
added a commit
to knockout/tko
that referenced
this pull request
May 14, 2018
mbest
added a commit
that referenced
this pull request
May 21, 2018
…r regarding destroyed items. See #2324
mbest
added a commit
that referenced
this pull request
May 21, 2018
mbest
added a commit
that referenced
this pull request
May 21, 2018
Also document computed's disposeWhenNodeIsRemoved with other disposal info.
brianmhunt
added a commit
to knockout/tko
that referenced
this pull request
Jun 15, 2018
* yarn) Update the packages so rollup can be used in each package directory
* tko) Fix export of `when`
* knockout) Add knockout proper package, plus somewhat modified spec from 3.5
See packages/knockout/spec/README.md
* Fix `jasmine.Clock.useMockForTasks`
* Expand tests to all `spec/` subdirectories
Also allow package.json’s `karma.files` to overload the default (by e.g. making them watchable)
* Fix semicolon hoisting variables to global scope
* spec) Remove duplicate import
* Upgrade devDependencies to latest
* karma) Fix relative `__dirname` import
* rollup) Better re-use of the path replacement
* observableArray - fix `compareArrays` tests
* npm) Update to latest packages, add globals, use .es6 knockout for testing
If we use `dist/knockout.js` for testing the knockout package, then we have to recompile every single dependency. If we attempt to link directly to the files `src/index.js` as we do for the .es6 variants then Typescript does not inject its tslib dependencies.
So we test off of `dist/knockout.es6.js`. One still has to recompile knockout to re-test every change.
* expose `ko.selectExtensions`
* knockout/templating) Fix a variety of template-related tests
* Down to 76 failing tests; fixes for templating and expressionRewriting behaviors
* Fix tests with dependencyDetection, postJson, and $context.ko (62 fail)
See `testing.md` for details
* tko.utils.functionRewrite) Add a basic util for backwards compat with `funciton () {}` in binding strings
* Add Backers.md for Pateron support
* parser) Add basic function rewriter to the default parser
Adds option `bindingStringPreparsers`
* build) Respect a `—es6` option to `yarn build` to speed up compilation
* string) Remove legacy `stringifyJson`
* #56 ) Break out common elements of knockout/tko compilation to `tko.builder`
This may have caused a couple regressions in tests, but they should be easy to find.
* packages) Fix configs for tko.functionRewrite and tko.builder
* rollup) Fix renamed config option
* export `utils.cloneNodes`
* Support ES2015 object initializer short-hand `{name}`
* function-rewrite) Rewrite arguments passed to lambdas
Note: arguments are not yet respected by the parser/interpreter.
* knockout) Put the `options in the correct place
* dev) Ignore `.vscode`
* parser) Add tests for calling lambdas created by the parser
Some lambdas may be passed into bindings.
* Add monkeypatch to fix the broken test system by breaking it
Time consuming & frustrating to diagnose.
* knockout) Fix tests related to `task` `scheduler` and `onError`
* Expose `ko.computedContext` as alias of `ko.dependencyDetection`
* Fix test for .length
* Update npm dependencies to latest
* MultiProvider) Respect the antiquated mechanism for providing `preprocessNode`
There’s certainly code out there that performs a `ko.bindingProvider.instance.preprocessNode = …`
* ko 3.5) Add `childrenComplete`
Cross-link knockout/knockout#2310
* Add `expressionRewriting` to `ko` global for backwards compatibility
* Make `ko.getBindingHandler` an overloadable function
* Prevent infinite recursion with `MultiProvider.instance`
* ci) Fix test wrt tko.bind
* dom/data) Add `getOrSet` function for dom data
* Make the async/component binding completion more explicit
* Make sure the MultiProvider returns an overloaded instances’ new nodes
* Implement details from knockout/knockout#2319
Probably we’ll be ripping all this out since it duplicates what’s in TKO already in terms of async binding handlers (but implementation details aside, this should still pass the KO 3.5 tests)
* Foreach parity re. knockout/knockout#2324
* knockout/knockout#2380 - ko.contextFor after applyBindingAccessorsToNode
* knockout/knockout#2378 Add tests and comply with spec
* knockout/knockout#2379 Fix style binding / compare to prior value
* Fix `arrayFirst` returning `undefined` when found element is falsy
Fixes #63
* Fix `ko.when` for promises & callbacks
* Respect the `createChildContextWithAs` option in the `with` binding
* Respect `createChildContextWithAs` in the native `foreach` binding
* Fix export of extenders
* Fix html parsing regex
* tko.utils) Fix circular reference
* knockout/spec) Update to `master` in KO
* knockout/knockout#2341 Respect `cleanExternalData` overload
@mbest - what do you think of the `addCleaner` and `removeCleaner` API? Or is that just overkill?
* Binding Provider) Better support for legacy `ko.bindingProvider.instance = …`
* template foreach) Fix the ko.bindingProvider being broke after if this test fails
* Add BindingResult class that exposes binding state
@mbest what do you think of this API?
Do you think `BindingResult` should include the node bound and binding context? That would be useful for debugging, and open up other useful possibilities - but might be outside the clean, core API.
* apply bindings) Add `rootNode` and `bindingContext` to the BindingResult
* Fix missing comma
* optionsBehaviors) Modify the tests to support lack of `function` support
* applyBindings) call `descendantsComplete` as expected
* tko.bind) Add a special BindingHandler `DescendantBindingHandler`
This should be the superclass of every class that bindings descendants, e.g. `if`, `with`, `foreach`, `template`.
* tko.bind) Add async binding context
This patch should bring parity with #2364, subject to the individual bindings (to be a separate commit)
* tko.bind) Fix error w/ virtual element binding
* tko.bind) Make sure `extend` doesn’t call `valueAccessor(value)`
In tko, `valueAccessor` with a parameter sets the value of the underlying data/observable. So we pass the value needed for extending an async context by binding the function’s `this`.
* Fix the builder failing to respect `ko.getBindingHandler` setter/getter
Also fix the upstream fix for a custom element tests misssing a `tick` and `template`.
* tko.bind/completion events) Use promises to verify that descendants completed binding
* backers) Update
* utils) Deprecate `proto` utilities, remove circular dependency
1. Fix the subscribable -> dependencyDetection -> … circular dependency
2. Always use `Object.setPrototypeOf`; this introduces a polyfill requirement/dependency for IE9.
* domNodeDisposal) Fix node cleaning test
Updated since merges from master.
* foreach) Fix `$ctx.$index` being overwritten on each update
It’s unusual & unexpected that `_contextExtensions` is being called multiple times; this may be a regression that needs investigation.
* tko.bind) Fix tests re. virtual element error
* tko.bind) Fix circular dependency for `BindingHandler` / `applyBindings`
* tko.bind) Reverting to `descendantsComplete` based on binding promises
@mbest - Note the failing tests and cross-linking #69 and #65.
* binding.template) Update double-unwrap test; disable “destroy” test
Note #69 re. foreachBehaviors.js:131
* util.parser) Skip argument exposure tests in lambdas
Noting for #65.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, the
foreachbinding has to compare the old and new arrays on each update. Using thearrayChangeevent will provide a performance improvement when using a known operation likepushwith large arrays.Unfortunately, in order to gain this benefit, we'll need to change the default behavior of
foreachto considerincludeDestroyedastrue.My question is: How many are using the
destroy/destroyAllmethods?