Skip to content

Update knockout #11269

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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 30, 2017
Merged

Update knockout #11269

merged 3 commits into from
Oct 30, 2017

Conversation

slackerzz
Copy link
Member

Upgrade knockoutjs to 3.4.2.
This is still the debug version because we use ko.utils.cloneNodes in engine.js
We have to wait until knockout/knockout#2248 gets released with knockout 3.5.
Alternatively we can refactor RemoteTemplateEngine.prototype.renderTemplateSource in engine.js and switch to the knockout standard release.

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@orlangur
Copy link
Contributor

orlangur commented Oct 6, 2017

What's the reasoning behind this change? There were any bugs affecting Magento?

@slackerzz
Copy link
Member Author

I tried to replace the debug release of knockout #11212, but then i halted because of ko.utils.cloneNodes in engine.js.
The reason to update the library is that between 3.3.0 and 3.4.2 there are a lot of bugfixes and and improvements, the list is quite long.

@orlangur
Copy link
Contributor

orlangur commented Oct 6, 2017

the list is quite long

This is irrelevant.

Main points to take into account:

  • how many things in Magento will be broken by update
  • how many bugs in Magento will get fixed as a result of knockout fixes
  • some new knockout features introduced (weakest point: using knockout too much does not seem to be a good approach)

I remember there was some bug with blur and autocompletion but last time I checked it was not fixed in knockout yet.

@orlangur
Copy link
Contributor

orlangur commented Oct 6, 2017

gets released with knockout 3.5

Is there any timeline for this by the way? In terms of fixing possible problems caused by update it may be worth do it only once 3.3 -> 3.5.

@slackerzz
Copy link
Member Author

slackerzz commented Oct 6, 2017

how many things in Magento will be broken by update

the only breaking change that i've found is knockout/knockout#1380 and i think i've fixed all the related problems in 216b595

how many bugs in Magento will get fixed as a result of knockout fixes

good point, hard to say

some new knockout features introduced (weakest point: using knockout too much does not seem to be a good approach)

the main new features are:

using knockout too much does not seem to be a good approach

well, i may agree with you, but this is what we have now, do we have alternatives?

I remember there was some bug with blur and autocompletion but last time I checked it was not fixed in knockout yet.

if you remember some specific case to test i'll take a look.

Is there any timeline for this by the way?

no dates, but it should not be so far away https://github.com/knockout/knockout/milestone/9

In terms of fixing possible problems caused by update it may be worth do it only once 3.3 -> 3.5.

if we upgrade to 3.4.2 now and fix what we need, we will have less problems to fix with 3.5
divide et impera! 😉

@orlangur
Copy link
Contributor

orlangur commented Oct 7, 2017

well, i may agree with you, but this is what we have now, do we have alternatives?

I mean just stick to subset of knockout features used instead of using it in full force :) But observables performance improvement by itself looks worth updating.

if you remember some specific case to test i'll take a look.

Here it is: #6826 (comment) Not sure if it is fixed on Magento side already.

if we upgrade to 3.4.2 now and fix what we need, we will have less problems to fix with 3.5
divide et impera! 😉

Yeah, for sure, just that it would be nice to execute full regression testing cycle only once instead of for both such updates

@slackerzz
Copy link
Member Author

Here it is: #6826 (comment) Not sure if it is fixed on Magento side already.

thanks, i'll do some tests

@slackerzz
Copy link
Member Author

@orlangur I've tested with Safari 11 and Chrome 61, i could't reproduce the issue you linked

@orlangur orlangur added this to the October 2017 milestone Oct 17, 2017
@orlangur orlangur self-assigned this Oct 19, 2017
@orlangur
Copy link
Contributor

@omiroshnichenko could you please take a quick look on these changes? Do you think they may be introduced in patch release or 2.3.0 is an earliest possible target?

@omiroshnichenko
Copy link
Contributor

@orlangur This update can affect many important places that not covered by tests and we don't know what hidden issue is present in new versions of knockout. So I think such update is not good for patch releases, better to have it in develop branch.

@orlangur
Copy link
Contributor

@omiroshnichenko thanks for your input. Yeah, I was quite surprised no functional test was broken running on Bamboo :)

@magento-engcom-team magento-engcom-team changed the base branch from develop to 2.3-develop October 20, 2017 15:32
@magento-team magento-team merged commit 216b595 into magento:2.3-develop Oct 30, 2017
magento-team pushed a commit that referenced this pull request Oct 30, 2017
magento-team pushed a commit that referenced this pull request Oct 30, 2017
[EngCom] Public Pull Requests - develop
 - MAGETWO-82942: Send email to subscribers only when are new #11604
 - MAGETWO-82752: Fixes translations by re-running the loadData for its correct locale #10913
 - MAGETWO-82460: Fix AcountManagementTest unit test fail randomly #11605
 - MAGETWO-82070: Update knockout #11269
@orlangur
Copy link
Contributor

@slackerzz big thanks for your contribution! Hope it won't break too much functionality not covered by automated tests but anyway the earlier we merge - the better 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants