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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix compatibility with Ember 3.13+ #462

Merged
merged 1 commit into from
Nov 23, 2019

Conversation

kobsy
Copy link
Contributor

@kobsy kobsy commented Oct 18, 2019

Summary

Ember 3.13 introduces an inaccessible WeakMap for framework-provided setters (to support tracked properties) keyed off of the owning object. Because this is the case, merely delete-ing an attribute from the context won't fully clean up the context; there's still a stale setter associated with the context and property name in the WeakMap. To address this, we would either need to separate the context passed down into the @ember/test-helpers setup methods from our persistent Mocha context (see my previous PR #461), or dig into Ember's private API to remove these setters from the WeakMap. While neither solution seems ideal, dealing with private API has massively less surface area to maintain, since any disposable context object would have to communicate its attributes and functions back and forth from the Mocha context to the Ember context -- a task that's perhaps not even possible to get correct for all use cases.

For the method for accessing the relevant private utility function, I drew from ember-cp-validations. While I don't love the idea of using private API, I'm at least comforted in knowing that other well-used addons also seem to do it. 馃槄

The tests are lifted directly from #461, and this PR would potentially replace it, addressing issue #430. Again, much thanks to the analysis done in the original issue and #450 & #460! 馃榿

@Turbo87
Copy link
Member

Turbo87 commented Oct 18, 2019

@rwjblue thoughts on this?

@ghost
Copy link

ghost commented Oct 18, 2019

@pzuraq I would be curious if you have any pointers for us to clear the state of MANDATORY_SETTERS. Or thoughts on a more correct way to solve the problem.

@ghost
Copy link

ghost commented Oct 18, 2019

@Turbo87 @kobsy what do you both think of opening and merging a separate PR with only the failing tests? It would seem best to me to decouple the failing tests with whatever solution we land on.

@ghost
Copy link

ghost commented Oct 21, 2019

I also tested this branch against our project with ember-source@3.13 and all the tests pass (without modifying the setting and getting logic).
After digging one question lingers:

  • why do we need to add additional teardown behavior when @ember/test-helpers teardownContext fulfills the same responsibility?

Used by test framework addons to tear down the provided context after testing is completed.
Responsible for:

  • un-setting the "global testing context" (unsetContext)
  • destroy the contexts owner object
  • remove AJAX listeners

@ghost
Copy link

ghost commented Nov 4, 2019

I think we need failing tests otherwise PR's like #471 give the false impression that ember-mocha works with the latest stable of ember-source.

@ghost
Copy link

ghost commented Nov 7, 2019

I verified the changes here ensure my test suite passes using 3.14.x. This is not surprising as 3.14 contains minor fixes not related to tracked properties behavior.

@leepfrog
Copy link

Same, this PR is what I needed to have my test suite working past 3.13.x (and currently works on 3.14 as well).

@Turbo87 Turbo87 changed the title Added step to tear down any mandatory setters created on the context Fix compatibility with Ember 3.13+ Nov 16, 2019
@Turbo87 Turbo87 added the bug label Nov 16, 2019
Ember 3.13 introduces an inaccessible WeakMap for framework-provided setters (to support tracked properties) keyed off of the owning object. Because this is the case, merely `delete`-ing an attribute from the context won't fully clean up the context; there's still a stale setter associated with the context and property name in the WeakMap. To address this, we would either need to separate the context passed down into the @ember/test-helpers setup methods from our persistent Mocha context, or dig into Ember's private API to remove these setters from the WeakMap. While neither solution seems ideal, dealing with private API has massively less surface area to maintain, since any disposable context object would have to communicate its attributes and functions back and forth from the Mocha context to the Ember context -- a task that's perhaps not even possible to get correct for all use cases. For the method for accessing the relevant private utility function, I drew from [ember-cp-validations](https://github.com/offirgolan/ember-cp-validations/blob/41158348fe8fc67ac3f3b3838dc967f5d1b7f383/addon/-private/ember-internals.js#L3-L6).
@Turbo87
Copy link
Member

Turbo87 commented Nov 23, 2019

I'll go ahead and merge this to unblock everyone :)

@rwjblue @pzuraq would still love feedback on this PR from you if you think that there is a better solution 馃檹

@Turbo87 Turbo87 merged commit 4da965e into emberjs:master Nov 23, 2019
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

3 participants