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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃帴 Incorporate ongoing PRs from Knockout/knockout #5

Closed
brianmhunt opened this Issue Sep 7, 2016 · 3 comments

Comments

Projects
None yet
2 participants
@brianmhunt
Member

brianmhunt commented Sep 7, 2016

From:

$ git log "v3.4.0"..HEAD --oneline

Complete

@brianmhunt brianmhunt changed the title from Evaluate PRs from Knockout/knockout to Incorporate ongoing PRs from Knockout/knockout Dec 4, 2016

@brianmhunt

This comment has been minimized.

Show comment
Hide comment
@brianmhunt

brianmhunt Dec 4, 2016

Member

@mbest 鈥 I've noticed a bunch of changes to KO 3.4 since the tko fork (good work, btw); could you please add a link to each of the PRs/merges here, so we can track them and they can be incorporated without having to search through the KO issues database. Otherwise I fear we may end up with some regressions from KO 3.4 to KO 4 (/TKO 1.).

Of course, where the change is apparent please feel free to incorporate directly.

Many thanks.

EDIT: I'm sure there's an automated way to get a list of all changes merged since 3.4.0. So this issue could be solved by figuring out the git command for that. :)

Member

brianmhunt commented Dec 4, 2016

@mbest 鈥 I've noticed a bunch of changes to KO 3.4 since the tko fork (good work, btw); could you please add a link to each of the PRs/merges here, so we can track them and they can be incorporated without having to search through the KO issues database. Otherwise I fear we may end up with some regressions from KO 3.4 to KO 4 (/TKO 1.).

Of course, where the change is apparent please feel free to incorporate directly.

Many thanks.

EDIT: I'm sure there's an automated way to get a list of all changes merged since 3.4.0. So this issue could be solved by figuring out the git command for that. :)

@brianmhunt brianmhunt changed the title from Incorporate ongoing PRs from Knockout/knockout to 馃帴 Incorporate ongoing PRs from Knockout/knockout Dec 4, 2016

@JD-Robbs

This comment has been minimized.

Show comment
Hide comment
@JD-Robbs

JD-Robbs Dec 13, 2016

Like git log "v3.4.0"..HEAD? I.e. just a simple listing of commits (hash, author, data date and description) since the v3.4.0 tag.

JD-Robbs commented Dec 13, 2016

Like git log "v3.4.0"..HEAD? I.e. just a simple listing of commits (hash, author, data date and description) since the v3.4.0 tag.

brianmhunt added a commit that referenced this issue Nov 9, 2017

brianmhunt added a commit that referenced this issue Nov 9, 2017

brianmhunt added a commit that referenced this issue Nov 10, 2017

brianmhunt added a commit that referenced this issue Nov 10, 2017

brianmhunt added a commit that referenced this issue Nov 10, 2017

brianmhunt added a commit that referenced this issue Nov 10, 2017

#5 253d39c 1fb6330 32bfa9d0
Also:
- some virtualElement clean-up
- various spelling fixes

brianmhunt added a commit that referenced this issue Nov 10, 2017

brianmhunt added a commit that referenced this issue Nov 10, 2017

brianmhunt added a commit that referenced this issue Dec 15, 2017

brianmhunt added a commit that referenced this issue Dec 15, 2017

brianmhunt added a commit that referenced this issue Dec 15, 2017

brianmhunt added a commit that referenced this issue Dec 15, 2017

brianmhunt added a commit that referenced this issue Dec 15, 2017

#5 241c26c
Also, use canonical function definitions

brianmhunt added a commit that referenced this issue Dec 15, 2017

brianmhunt added a commit that referenced this issue Dec 15, 2017

brianmhunt added a commit that referenced this issue Dec 15, 2017

brianmhunt added a commit that referenced this issue Dec 15, 2017

brianmhunt added a commit that referenced this issue Dec 15, 2017

brianmhunt added a commit that referenced this issue Dec 15, 2017

brianmhunt added a commit that referenced this issue Dec 15, 2017

brianmhunt added a commit that referenced this issue Dec 15, 2017

brianmhunt added a commit that referenced this issue Dec 15, 2017

brianmhunt added a commit that referenced this issue Dec 15, 2017

brianmhunt added a commit that referenced this issue Dec 16, 2017

brianmhunt added a commit that referenced this issue Dec 16, 2017

brianmhunt added a commit that referenced this issue Dec 16, 2017

brianmhunt added a commit that referenced this issue Dec 16, 2017

brianmhunt added a commit that referenced this issue Dec 16, 2017

brianmhunt added a commit that referenced this issue Dec 16, 2017

brianmhunt added a commit that referenced this issue Dec 16, 2017

brianmhunt added a commit that referenced this issue Dec 16, 2017

brianmhunt added a commit that referenced this issue Dec 18, 2017

brianmhunt added a commit that referenced this issue Dec 19, 2017

brianmhunt added a commit that referenced this issue Dec 19, 2017

#5 f9fe3d4 a32bd19
Also, standardJs applied throughout the test file (removed an eslint config-comment)

brianmhunt added a commit that referenced this issue Dec 19, 2017

brianmhunt added a commit that referenced this issue Dec 19, 2017

#5 04a8f84 554022b 5dc0ba0 01237e7 bb70d30
Note / FIXME the `with` binding is using a legacy handler owing to something being amiss in the deferUpdate handlers.

brianmhunt added a commit that referenced this issue Dec 19, 2017

brianmhunt added a commit that referenced this issue Dec 19, 2017

brianmhunt added a commit that referenced this issue Dec 19, 2017

#5 fd4ed82 b58e2b6
Note: The issue in question did not appear in tko i.e. the fix was not necessary in order for the tests to past.

brianmhunt added a commit that referenced this issue Dec 19, 2017

brianmhunt added a commit that referenced this issue Dec 19, 2017

brianmhunt added a commit that referenced this issue Dec 19, 2017

@brianmhunt

This comment has been minimized.

Show comment
Hide comment
@brianmhunt

brianmhunt Dec 19, 2017

Member

@mbest - Thanks for all these PRs; I'll be caught up to the latest ones soon.

A couple notes I wanted to share:

  1. Comments around new code have really helped; not just for their content, but especially because they generally provide unique searchable text that makes it easier to pare down where the changes are in the code-base.

  2. I'm getting a bizarre error using the with binding, notably if you look at tko's WithBindingHandler in with.js, and the WithBindingHandlerFailsInexplicably, there's something amiss in that the latter fails the test Should update "with" binding before descendant bindings in asyncBindingBehavior.js. However the topmost calls they make appear to be identical, and all other tests appear to pass. I dug down and there seem to be a lot more calls to the _evalIfChanged with the version that works. The essence of the problem occurring in the test appear to be that the observables are not being triggered in the correct order.

You can try it out by running npm run watch in the tko.binding.if directory, and compare them by renaming the export (i.e. changing WithBindingHandlerFailsInexplicably to WithBindingHandler and renaming the other so there's no name conflict).

I'll keep staring at it, but since it seems to be deep in the deferUpdate code I thought you might have some insight. . I was using createStaticChildContext instead of createChildContext

Member

brianmhunt commented Dec 19, 2017

@mbest - Thanks for all these PRs; I'll be caught up to the latest ones soon.

A couple notes I wanted to share:

  1. Comments around new code have really helped; not just for their content, but especially because they generally provide unique searchable text that makes it easier to pare down where the changes are in the code-base.

  2. I'm getting a bizarre error using the with binding, notably if you look at tko's WithBindingHandler in with.js, and the WithBindingHandlerFailsInexplicably, there's something amiss in that the latter fails the test Should update "with" binding before descendant bindings in asyncBindingBehavior.js. However the topmost calls they make appear to be identical, and all other tests appear to pass. I dug down and there seem to be a lot more calls to the _evalIfChanged with the version that works. The essence of the problem occurring in the test appear to be that the observables are not being triggered in the correct order.

You can try it out by running npm run watch in the tko.binding.if directory, and compare them by renaming the export (i.e. changing WithBindingHandlerFailsInexplicably to WithBindingHandler and renaming the other so there's no name conflict).

I'll keep staring at it, but since it seems to be deep in the deferUpdate code I thought you might have some insight. . I was using createStaticChildContext instead of createChildContext

brianmhunt added a commit that referenced this issue Dec 19, 2017

brianmhunt added a commit that referenced this issue Dec 19, 2017

brianmhunt added a commit that referenced this issue Dec 19, 2017

brianmhunt added a commit that referenced this issue Dec 19, 2017

brianmhunt added a commit that referenced this issue Dec 19, 2017

brianmhunt added a commit that referenced this issue Dec 19, 2017

@brianmhunt brianmhunt closed this Dec 20, 2017

brianmhunt added a commit that referenced this issue Dec 20, 2017

brianmhunt added a commit that referenced this issue Dec 20, 2017

brianmhunt added a commit that referenced this issue Dec 20, 2017

brianmhunt added a commit that referenced this issue Dec 20, 2017

brianmhunt added a commit that referenced this issue Dec 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment