-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix(ember): use _backburner
if it exists
#4603
fix(ember): use _backburner
if it exists
#4603
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR - appreciate the help :)
We have some Next 12 tests failing on master that we are looking to fix, so you can ignore those for now!
if (run.backburner) { | ||
return run.backburner; | ||
} | ||
return _backburner; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this change not all code paths here will return a value - so getBackburner()
might return undefined now. Can we get around this somehow? Or do we have to do larger refactors for the usage of the getBackburner
function?
This is why https://github.com/getsentry/sentry-javascript/runs/5235458452?check_suite_focus=true is failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I am not knowledgable enough in the history of Ember, but I believe that if _backburner
does not exist, it means we are on an older version of Ember and thus run.backburner
exists. We could thus remove the condition around run.backburner
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@k-fish may be able to help us here
Hey, please rebase this branch as we’ve fixed the test failures on master. |
`run.backburner` is deprecated and creates errors now when used. This commit fixes the issue by using the `_backburner` if it exists (which is the case in recent versions of Ember), or `run.backbackburner` if not (which is the case in older versions of Ember).
4cef50a
to
107f02d
Compare
I updated the code by always returning a value, and rebased to the latest master as asked. |
} | ||
return _backburner; | ||
|
||
return run.backburner; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this is still failing CI as run.backburner
can be undefined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just took a look, and agree that _backburner should be first so newer Ember versions won't be using the old way of accessing it. We'd likely want to stay defensive here since we really don't want Sentry to be the cause of breaking someones app in an initializer on any version of Ember.
On that note, I'd keep the undefined check, and make a simple empty mock object that matches the usage, something like this:
function getBackburner(): Pick<ExtendedBackburner, 'on' | 'off'> {
if (_backburner) {
return _backburner;
}
if (run.backburner) {
return run.backburner;
}
return {
on() {},
off() {},
};
}
@k-fish I followed your feedback and return a mock in the final case. |
_backburner
if it exists
run.backburner
is deprecated and creates errors now when used and when ember-cli-deprecation-workflow is in your dependencies.This commit fixes the issue by using the
_backburner
if it exists (which is the case in recent versions of Ember), orrun.backbackburner
if not (which is the case in older versions of Ember). Thus,run.backburner
is not called if_backburner
is present and thus does not create a deprecation warning.This fixes the issue reported here event after the PR was merged.