Open kjhangiani opened 6 years ago
cowboyd
commented
6 years ago Sorry for the confusion Kevin! And thank you for taking the time to stop and submit a detailed issue in order to make the entire project better for everyone.
Clearly you've brought a great deal of context to this already. What can I do to help facilitate a pull request that contains the Changelog you would like to see rather than the misleading one that we have today?
kjhangiani
commented
6 years ago @cowboyd I'm still working through some of the issues - feel like I have a pretty good handle on the codebase now, but haven't had time to finish up the work. For the time being, I had to hotfix 2.0.2 (where we were currently at) to include the contains/includes deprecation fix, but otherwise didn't change any behavior.
There are a couple issues that are making this upgrade troublesome, but I think it should be possible to fix in such a way that the upgrade process will go a lot smoother for any other users in a similar situation
1) The ember-source problem in index.js makes it impossible to upgrade emberx-select to intermediate versions on later versions of ember. This fix is currently only in 3.0.0+
2) The deprecation for contains/includes really slows down dev sites, and this fix is also only in 3.0.0+
3) With the change to support one-way bindings, combined with this issue: https://github.com/thefrontside/emberx-select/issues/151, the deprecation issue is greatly amplified because of the commit (as specified in the ticket) here: https://github.com/thefrontside/emberx-select/commit/ea9522537cd4a4eb0b6c46f283bb13f0f1989860
This change causes action to fire on init for every selectpicker, regardless of whether 2way or 1way bindings is used.
After playing around with this, I am of the opinion that this change is rather significant, and I actually disagree with it. I understand the purpose of it, but this fundamentally changes the behavior of the select (for both 2way and 1way).
This actually causes a discrepancy in how emberx-select behaves vs a native select. A native select does not fire the change event on init, only when changed - however, this change makes it so that action, which is effectively a default change event fires both on init, and on changes.
4) 3 actually ends up causing a lot of problems, we have to go one by one and alter the logic considerably on each usage of emberx-select. However, there is a workaround, in that instead of using action, one can instead bind to on-change, which would fire in the same manner as older versions (and maintain behavior with a native select). However, again, in the 2.x branches, the action handlers have a different argument order, and are named differently, which means it has to be refactored again when upgrading to 3.x
What I would propose is the following (and I will attempt to make a PR this week..)
Release v2.2.4, with the following
on-change should be used in place of action if users do not want the init actionPR against master with the following
on-init that also fires at the same time as action in the setup process. this would allow usage of action if users want the same action to fire on init as well as change, or on-init and on-change if different events are required. (this event can also be added to 2.2.4 if it makes sense to do so)Sorry for the long message - unsure if anyone else is in the same situation as us, but this seems to be the most reasonable pathway for us to have a stepping stone between 2.0.2 and 3.x
cowboyd
commented
6 years ago Long, cogent messages like this one rule! So no need to apologize.
Having on-init and on-change events be distinct seems very reasonable, and in line with what we do on other libraries.
kjhangiani
commented
6 years ago @cowboyd I made some progress on the first half of my proposal today, and created a branch here:
https://github.com/soxhub/emberx-select/tree/v2.2.4
Here is the diff: https://github.com/soxhub/emberx-select/compare/v2.2.3...v2.2.4
What branch should I PR this against?
So far, I'm about halfway through our app and the migration process has been progressing smoothly. I have found a few places where it is difficult to remove the 2-way bindings, but this branch is allowing me to at least proceed with the other changes (contextual, on-change etc) to make the 3.x transition easier later.
We are in the process of upgrading emberx-select from 2.0.2 to 3.x, but because we still have some 2-way bindings, are attempting to do it in phases
The main issues we are trying to address are : 2 way bindings, contextual components, and fixing the
containstoincludesdeprecation warning. Our goal was to jump up to 2.2.3 (where 2 way is still supported, and the deprecation is fixed, and contextual components are supported), then migrate/test and eventually update to 3.xHowever, it seems like a lot of the fixes we expected to find in 2.2.3 as described in the changelog are not actually in 2.2.3, but are in 3.0.0. Perhaps the changelog could be updated to more accurately reflect these fixes?
This caused us a bunch of confusion until we were able to figure out why the expected fixes were not in place.
For reference, see the 2.2.2 -> 2.2.3 diff here: https://github.com/thefrontside/emberx-select/compare/v2.2.2...v2.2.3
And compare it to the 2.2.3 -> 3.0.0 diff here: https://github.com/thefrontside/emberx-select/compare/v2.2.3...v3.0.0
A number of the fixes that are listed under 2.2.3 in the changelog (
contains->includes, fixes forember-sourcein the version checker) are listed under 2.2.3, but are actually merged into 3.0.0We ended up forking the library and creating a hotfix branch to fix the specific issues we had problems with so we can smooth our migration to 3.x (https://github.com/soxhub/emberx-select/tree/v2.2.3-hotfix)