Skip to content
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

changelog for 2.2.3 appears to be incorrect #202

Open
kjhangiani opened this issue Nov 30, 2017 · 4 comments
Open

changelog for 2.2.3 appears to be incorrect #202

kjhangiani opened this issue Nov 30, 2017 · 4 comments

Comments

@kjhangiani
Copy link

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 contains to includes deprecation 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.x

However, 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:
v2.2.2...v2.2.3

And compare it to the 2.2.3 -> 3.0.0 diff here:
v2.2.3...v3.0.0

A number of the fixes that are listed under 2.2.3 in the changelog ( contains -> includes, fixes for ember-source in the version checker) are listed under 2.2.3, but are actually merged into 3.0.0

We 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)

@cowboyd
Copy link
Collaborator

cowboyd commented Dec 5, 2017

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
Copy link
Author

@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: selectionChanged loop in 2.2.2 #151, the deprecation issue is greatly amplified because of the commit (as specified in the ticket) here: ea95225

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.

  1. 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

  • fix the ember-source issue
  • fix the contains/includes deprecation
  • fix the changelog entries for 2.2.3, and add 2.2.4
  • make changes to the event names and arguments such that they match up with 3.0.0 (rename events, reorder arguments)
  • update readme to indicate that to maintain old behavior, on-change should be used in place of action if users do not want the init action

PR against master with the following

  • update changelog for 2.2.3, add changelog for 2.2.4
  • add an event 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
Copy link
Collaborator

cowboyd commented Dec 6, 2017

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 added a commit to soxhub/emberx-select-backup that referenced this issue Dec 7, 2017
- this also corrects the changelog from 2.2.3 to match the release
- see: adopted-ember-addons/emberx-select@v2.2.2...v2.2.3
- see: adopted-ember-addons#202
kjhangiani added a commit to soxhub/emberx-select-backup that referenced this issue Dec 7, 2017
- see: adopted-ember-addons#202
- adds support for `on-init` that fires when the select is constructed
	- since 2.2.2, `action` is fired by default on both init and change
	- this can be a breaking change for some apps, so separating `on-init` allows users to use `on-change` / `on-init` or `action` depending on when they need events to fire
@kjhangiani
Copy link
Author

@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.

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

No branches or pull requests

2 participants