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

Upgrade ember-cli/ember & include glimmer 2 regression fix from Alex Hancock #52

Merged
merged 7 commits into from
Dec 13, 2017

Conversation

toranb
Copy link
Collaborator

@toranb toranb commented Dec 11, 2017

Working out a few things and noticed we had ember-cli 2.7 so I've done a big upgrade/ also brought a more strict look at the ember try config

1.13
2.12
2.16
2.17

@toranb
Copy link
Collaborator Author

toranb commented Dec 11, 2017

@MiguelMadero looks like ember 1.13 and ember-try (vNext) isn't playing nice. You have any example config using ember-cli that's solid so we can keep it in the matrix? Here is the error I see right now - ENV related

screen shot 2017-12-10 at 9 03 49 pm

cc @alexhancock

@toranb
Copy link
Collaborator Author

toranb commented Dec 11, 2017

Worst case we cut a 0.2 release and drop 1.13 from the ember-try config for this addon. Not sure how many ember 1.13 users we have but honestly this change shouldn't break them. Going forward how many addons are truly supporting 1.13? I saw a survey from Miguel (ember power select) and it seems like 2/3% of people who responded are on ember < 2.0

@toranb
Copy link
Collaborator Author

toranb commented Dec 11, 2017

@MiguelMadero I did confirm that this fix does indeed solve the hot reloader regression that we suffered in the post glimmer v0.22 update. I manually tested this with ember 2.13, 2.14, 2.16 and 2.17 using the repository below.

https://github.com/toranb/hacking-hot-reloader-again

@alexhancock

note: this did reveal a client side (live reload) bug that occurs occasionally if you don't reload the page w/ empty cache + hard reload (more on this later)

@alexhancock
Copy link
Collaborator

Cool! I see your change updates ember-cli-babel as well, which is something I was going to request in a separate change.

I look forward to seeing this all merged.

@MiguelMadero
Copy link
Collaborator

@alexhancock @toranb this looks great, thanks for all the work on this! I have been busy with other projects and unable to keep up with some of the goals I had for this. I'm really glad to see you moving it forward.

I'm not sure why it fails. I have other projects that work with 1.13 and use new-ish versions of ember-try, but I didn't find anything different on the configuration. I suggest we merge it as is and we deal with adding 1.13 back on another PR, if needed.

@toranb toranb changed the title Ember Upgrade/ including new try config and fix from Alex Upgrade ember-cli/ember & include glimmer 2 regression fix from Alex Hancock Dec 12, 2017
@toranb
Copy link
Collaborator Author

toranb commented Dec 12, 2017

@MiguelMadero I'm much closer to releasing this. As I included ember 2.18 beta it became clear targetObject was deprecated (in favor of target). I tested this manually with ember 2.13/2.17 but found 1 test in the test suite broke. I'm almost certain this issue is due to action bubbling changes with target vs targetObject

<h2>component-with-actions using classic actions</h2>
{{component-with-actions classNames="component-with-actions--classic" componentAction="routeAction"}}

This components internals

export default Ember.Component.extend({
  layout: hbs`
    <a {{action 'myAction' }}>Some button</a>
  `,
  actions: {
    myAction () {
      this.sendAction('componentAction');
    }
  }
});

And the error thrown as a result of clicking that

Assertion Failed: <dummy@component:component-with-actions::ember318> had no action handler for: routeAction"

Any pointers?

@MiguelMadero
Copy link
Collaborator

MiguelMadero commented Dec 12, 2017 via email

@alexhancock
Copy link
Collaborator

@toranb

So to clarify, it seems like what is happening is component-with-actions is getting the string and trying to call routeAction on itself when you call sendAction, but that action obviously does not exist. When you render the component...

{{component-with-actions classNames="component-with-actions--classic" componentAction="routeAction"}}

... you could try using the action helper, like so...

componentAction=(action "routeAction")

... instead of just passing the string like ...

componentAction="routeAction"

I can't think of why that would make any difference (it should in theory work both ways), but that is how they do it in these docs, so it might be worth trying.

In this ember-twiddle, both ways work, but it might be some difference in Ember version.

@toranb
Copy link
Collaborator Author

toranb commented Dec 12, 2017

@MiguelMadero sadly I'm fairly booked during the day so pairing is challenging. I will dive further into the changes ember 2.18 introduced that took away targetObject

@alexhancock the challenge here is that everything worked for both ember 2.16/2.17 but as of 2.18 targetObject goes away. I replaced this with target instead and this is when the regression pop'd up. I will dive into "why" a bit more later in the week but I'm open to others help if you've got any spare cycles outside of work/ etc

@toranb
Copy link
Collaborator Author

toranb commented Dec 13, 2017

After some thought I decided this PR was big enough as-is and could land without the ember 2.18 beta work for now. I'll close this out and cut a release so anyone interested can check this project out again. I'll open an issue for the 2.18 work and if anyone has the time to jump in I'd appreciate it

I'll document what I know and touch base after the holiday to discuss when we can solve the deprecated targetObject prop we use right now

@toranb toranb merged commit 7c9591e into master Dec 13, 2017
@toranb
Copy link
Collaborator Author

toranb commented Dec 13, 2017

I've published v0.1.9 and confirmed it's working with ember 2.17

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.

None yet

3 participants