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

Update the assignment of this.args to reflect the arg proxy #201

Merged

Conversation

pzuraq
Copy link
Member

@pzuraq pzuraq commented Jul 19, 2019

The refactor to use a proxy for tracking individual updates to args
requires us to make some changes here in how we assign arguments. Also
adds the updateHook option to capabilities. This solution should be
compatible with Ember pre and post enabling the tracked canary flag,
but can be made better with ember-compatibility-helpers once we have
a version of Ember to target.

@pzuraq pzuraq force-pushed the bugfix/update-args-assignment-for-proxy branch 3 times, most recently from b012aae to ab4a819 Compare July 22, 2019 19:09
@rwjblue
Copy link
Member

rwjblue commented Jul 24, 2019

can be made better with ember-compatibility-helpers once we have a version of Ember to target

You should be able to use gte('3.13.0-alpha.0') already.

@rwjblue
Copy link
Member

rwjblue commented Jul 24, 2019

Looks like the CI failure is a bunch of typescript errors when ran against canary builds of ember-source.

@rwjblue
Copy link
Member

rwjblue commented Jul 24, 2019

@tomdale - Isn't this canary failure the same as you hit over in #202? I thought we chatted about it and that you had path forward, but I can't recall what it was. Do you recall?

@pzuraq
Copy link
Member Author

pzuraq commented Jul 24, 2019

@rwjblue I wanted to be a bit more sure before we pinned it here, it'll be annoying if we release a version and it breaks later, but I can also add conditional code if you prefer. Definitely can put the test in the conditional at least.

As for the test failures, there's a legitimate one in there, it's caused by the fact that CPs can't work with the args proxy yet. Fix is here: emberjs/ember.js#18217

@pzuraq pzuraq force-pushed the bugfix/update-args-assignment-for-proxy branch 2 times, most recently from dd400ae to 4956620 Compare August 1, 2019 18:58
The refactor to use a proxy for tracking individual updates to args
requires us to make some changes here in how we assign arguments. Also
adds the `updateHook` option to capabilities. This solution should be
compatible with Ember pre and post enabling the tracked canary flag,
but can be made better with ember-compatibility-helpers once we have
a version of Ember to target.
@pzuraq pzuraq force-pushed the bugfix/update-args-assignment-for-proxy branch from 4956620 to 82f4863 Compare August 8, 2019 00:22
@tzellman
Copy link

tzellman commented Aug 8, 2019

Great work here!

There is currently an odd proxy-related error that occurs on ember canary (or 3.13.x), and a few of us believe that this will fix the error. See this example repo which highlights the problem: https://github.com/justinross/e-octane-bug

@rwjblue rwjblue merged commit 7829121 into glimmerjs:master Aug 8, 2019
@rwjblue rwjblue changed the title [BUGFIX] Updates the assignment of args to reflect the arg proxy Update the assignment of this.args to reflect the arg proxy Aug 8, 2019
@rwjblue rwjblue added the bug label Aug 8, 2019
@tzellman
Copy link

tzellman commented Aug 8, 2019

Thank you for the quick response @rwjblue and for the changes @pzuraq!

🎉

@vitch
Copy link

vitch commented Aug 15, 2019

Thanks for this fix!

We had the same problem demonstrated in the repo above:

TypeError: 'ownKeys' on proxy: trap returned extra keys but proxy target is non-extensible

And as discussed here updating @glimmer/component to 0.14.0-alpha.11 and updating ember-source to https://s3.amazonaws.com/builds.emberjs.com/canary/shas/e8a6420f39d0a6a6f67a85f62f30fbb5c4339507.tgz (from 3.13.0-beta.2) fixed the issue.

I'm wondering what it is about the canary that makes it work. Are there changes in ember-source or features that need to be enabled for this to work?

@pzuraq
Copy link
Member Author

pzuraq commented Aug 15, 2019

There were changes in ember-source yes. They are being backported to the next beta cycle.

@pzuraq pzuraq deleted the bugfix/update-args-assignment-for-proxy branch August 15, 2019 18:06
vitch pushed a commit to vitch/ember-octane-test that referenced this pull request Aug 16, 2019
Per [this PR](glimmerjs/glimmer.js#201), this is meant
to fix the bug but it doesn't with the latest `ember-source`
@vitch
Copy link

vitch commented Aug 16, 2019

It looks like the relevant fix in ember-source regressed in this range.

Or I'm misunderstanding what this was meant to fix. I put together a a reproduction here - should that work?

@vitch
Copy link

vitch commented Aug 16, 2019

Discussed on discord and fixed by @pzuraq in ember.js#18274 - thanks!

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

Successfully merging this pull request may close these issues.

None yet

4 participants