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: ignore mirage directory for avoid-leaking-state-in-ember-objects #1004

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

BarryThePenguin
Copy link
Contributor

@BarryThePenguin BarryThePenguin commented Nov 11, 2020

Unsure if this is the right approach for mirage.. but I thought I'd see what others think 馃挱

This code in mirage..

import { Factory } from 'ember-cli-mirage'

export default Factory.extend({
  foo: []
})

..has the same issues as the similar code in ember that this rule is trying to prevent..

export default Foo.extend({
  items: [],
});

..but the consequences aren't as bad, as mirage isn't normally run in production.

Though there is still the risk of leaking state in tests?

Fixes #202
Fixes #214

@bmish bmish requested a review from rwjblue November 11, 2020 21:06
@bmish
Copy link
Member

bmish commented Nov 19, 2020

I'd like to get @rwjblue's opinion on this.

@jaydgruber
Copy link
Contributor

Suggestion - gate this behind a config flag? Maybe default to true is fine.

We've run into leaking state in tests plenty of times. Granted we've got ~4k tests running very split in ember exam, so some of that's on us 馃槩. Not currently using mirage, but I would think some teams would like to be able to config this if they do have test leak problems.

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