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

Don't rely on Ember.EmberInspectorDebugger #1760

Merged
merged 1 commit into from
Nov 2, 2021

Conversation

sandstrom
Copy link
Contributor

@sandstrom sandstrom commented Oct 22, 2021

Closes #1758

No idea if this is the best way of solving it.

But since we're moving away from the Ember global a change such as this seemed reasonable, and respects the existing code (this change is minimal), since window.EmberInspector already existed.

Deleted Ember.EmberInspectorDebugger altogether, since it's not used elsewhere and I cannot find any code in Github (across all projects) that rely on it.

https://github.com/search?q=EmberInspectorDebugger&type=code

@RobbieTheWagner
Copy link
Member

Thanks @sandstrom! You're right, I am not sure Ember.EmberInspectorDebugger is used anywhere. I think the idea was it would be stored on Ember rather than the window, but I am not sure why we were doing both. It seems like that code was from many years ago, so I think changing it is fine.

@sandstrom
Copy link
Contributor Author

@rwwagner90 can you trigger re-run of actions? https://github.com/emberjs/ember-inspector/runs/3975937135?check_suite_focus=true seems like it's unrelated to this code, no? tests are only failing on beta

@RobbieTheWagner
Copy link
Member

@sandstrom I'm seeing the same issues in #1759

Seems like something broke with ember test helpers in general

@RobbieTheWagner
Copy link
Member

@sandstrom I think that issue has been fixed now. Can you please rebase with master?

@sandstrom
Copy link
Contributor Author

@rwwagner90 Alright, new attempt 😄

@sandstrom
Copy link
Contributor Author

@rwwagner90 Do you know anyone with write-access who can merge this?

@RobbieTheWagner
Copy link
Member

@sandstrom I have write access, I'm trying to get feedback from @chancancode on if there are any drawbacks to this change.

@RobbieTheWagner RobbieTheWagner merged commit 8be5d8f into emberjs:master Nov 2, 2021
@sandstrom sandstrom deleted the patch-1 branch November 2, 2021 09:37
@chancancode
Copy link
Member

Seems fine to me

patricklx pushed a commit to patricklx/ember-inspector that referenced this pull request Sep 19, 2022
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.

Cannot read properties of undefined (reading '_application'), Maximum call stack size exceeded
3 participants