-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[ui] When a purged/404-ing job is detected, boot the user out of that job and back to the index #17915
[ui] When a purged/404-ing job is detected, boot the user out of that job and back to the index #17915
Conversation
9f85c4a
to
56974ba
Compare
Ember Test Audit comparison
|
@@ -3,4 +3,5 @@ | |||
SPDX-License-Identifier: MPL-2.0 | |||
~}} | |||
|
|||
{{did-update this.notFoundJobHandler this.watchers.job.isError}} |
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.
Using an Ember Render Modifier stand-alone; that is, not attached to any particular nested element.
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 for my education, the discussion here suggests these aren't necessarily a good idea to use. Why might that be different for us?
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.
Probably 2 reasons why it's advocated against:
- Ember (and other frameworks that came out around the same time) had a big habit of using the "observer" pattern for derived state in codebases during their early releases. This often ended badly as you'd end up with many components that observed and permuted their own data based on something changing elsewhere, but not being strictly dependent on it. This would cause side-effects, out-of-sync data, etc. A good response to rethinking this pattern in Ember is up at https://discuss.emberjs.com/t/a-better-observer-pattern-in-ember/18589/2 — but basically, at a framework level, Ember started advocating against Observers to try to discourage the side-effectishness that comes with it. This
{{did-update}}
is more or less an observer. - A smaller reason that's explicitly mentioned is the ability for us to abstract
did-insert
/did-update
modifiers into our own custom modifiers. This helps us write more nuanced and performant modifiers that can be tailored to our app logic. The Keyboard Shortcut modifier is a custom one that could've been adid-insert
but was specific enough that a custom modifier was warranted.
My take on the reason this is fine for us here: the majority of cases where this is advocated against have to do with a deliberate user action being responded to. As in, 1. user presses button, 2. process kicks off, [...], n. something needs to happen based on suddenly-changed state.
did-update
handles this stuff and lets you forget about the process between the user action and the derived state change. But a better model would be to have the initial user action kick off a process directly (via Promise
or async/await
pattern if needed) that would handle the resultant needed changes.
Problem here is: this is not a change that's driven by a user action that we can observe; this is driven by an external factor (remote job purge). Our best mechanism for reacting to changes here is to say "Wait for this specific thing to change", as there's nothing in-application that could kick this off.
Another good place where we use {{did-update}}
-style patterns is on, for example, recomputing the axis ticks of our charts when the user resizes their browser. There's no particular in-app action they've taken where we could kick a process off, but we can wait and pounce on browser dimension changes and kick off changes based on them.
This pattern will probably be a major driver of the UI for the event stream, when that gets built, as well.
b97958a
to
aa95317
Compare
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.
I've pulled this down locally and played around with it. LGTM 👍
@@ -3,4 +3,5 @@ | |||
SPDX-License-Identifier: MPL-2.0 | |||
~}} | |||
|
|||
{{did-update this.notFoundJobHandler this.watchers.job.isError}} |
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 for my education, the discussion here suggests these aren't necessarily a good idea to use. Why might that be different for us?
watchRecord
watcher util to accept ashouldSurfaceErrors
param. This is experimental, but makes it so that elsewhere we can checkthis.model.watchers.{}.isError
, where before it was being yielded but not set on the watcher itself.Resolves #17757