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

setting this.render is broken in 0.7.21 #359

Closed
kellyselden opened this issue Apr 13, 2018 · 9 comments
Closed

setting this.render is broken in 0.7.21 #359

kellyselden opened this issue Apr 13, 2018 · 9 comments

Comments

@kellyselden
Copy link
Member

https://github.com/kellyselden/ember-test-setup/blob/master/addon-test-support/-setup-render.js#L9 was working on the assumption in 0.7.20 that I could override this.render. It was broken in #349. I understand why it was changed, but it sucks I can't have my desired API anymore.

@Turbo87
Copy link
Member

Turbo87 commented Apr 13, 2018

I'm somewhat surprised that that API has ever worked :D

@kellyselden
Copy link
Member Author

Any opinions on this? Would you consider reverting the read-only assertion?

@rwjblue
Copy link
Member

rwjblue commented Apr 17, 2018

IMHO, using this.render from this library should actually be deprecated. Folks should be import { render } from '@ember/test-helpers'...

@kellyselden - That change was added for good reason (folks were clobbering things in a way that left them in a horribly broken state and they had no clue). I don't really know how to weigh the volume of that vs the volume of the users of ember-test-setup. If we need to treat this as a regression we should allow clobbering this.render but emit a deprecation/warning when it happens...

@kellyselden
Copy link
Member Author

@rwjblue You mean a deprecation when this.render from this library is run, not when this.render is set, correct?

@rwjblue
Copy link
Member

rwjblue commented Apr 17, 2018

@kellyselden - I think I meant both? But the important one is that we deprecate using this.render from this lib...

@kellyselden
Copy link
Member Author

@rwjblue Hmm I guess that would be ok. So then users of ember-test-setup would get deprecations until this.render is removed from here, then the warnings would disappear?

@Turbo87
Copy link
Member

Turbo87 commented Apr 18, 2018

FWIW I'm pretty opposed to reverting this. We have implemented these guards for a good reason and I don't think people should ever override the functions that we set up here. If ember-test-setup did that, then we should think about a better API for that addon, that does not involve overwriting functions from @ember/test-helpers.

@rwjblue
Copy link
Member

rwjblue commented Apr 18, 2018

I tend to agree @Turbo87. IMHO a better (more aligned API) would be for users of ember-test-setup to use an imported render helper from that addon...

@Turbo87
Copy link
Member

Turbo87 commented May 18, 2018

@kellyselden I still think you shouldn't overwrite this.render(), but I guess #374 may have opened a loophole for you. Closing this issue now.

@Turbo87 Turbo87 closed this as completed May 18, 2018
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 a pull request may close this issue.

3 participants