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

Error reporting #26

Closed
justincy opened this issue Nov 28, 2017 · 7 comments
Closed

Error reporting #26

justincy opened this issue Nov 28, 2017 · 7 comments

Comments

@justincy
Copy link
Member

When a 4xx or 5xx error is encountered we need to display the error to the client. An easy way to get an error is to change the gender.

@justincy
Copy link
Member Author

I can think of two different scenarios:

  1. Global event handler for something like toast notifications
  2. Element specific error handling

If errors were always emitted from the element that generated the request then we could achieve both cases (a global event handler would listen for error events on any element that contains all fs elements, such as the app or the body).

fs-request already emits error and fs-request-error events but those are only for network errors, not HTTP errors. Also, some elements use fs-request in memory only without ever attaching it to the DOM so events don't propagate. Thus we need to do two things:

  1. Emit a new HTTP error event
  2. Make sure all in memory usages of fs-request somehow propagate errors

@justincy
Copy link
Member Author

justincy commented Nov 28, 2017

I can think of two ways to make sure in memory usage of fs-request propagates errors:

  1. Stop using fs-request without attaching it to the DOM.
  2. Provide a delegateEvents() method that allows you to specify which element the events are emitted on so that fs-request could be configured to emit events on it's containing element.

I'm not a fan of option 2 so we'll go with 1.

There are some situations where we are able to keep a single fs-request element in the DOM and reuse but for whatever reason just chose to not do that. Option 1 is trivial in those cases.

There are other situations, such as in mixins, where relying on an element being in the DOM at all times is a little more precarious. These cases can be handled by dynamically attaching it to the element's DOM before the request is executed and then removing it later if desired.

@justincy
Copy link
Member Author

justincy commented Nov 28, 2017

@justincy
Copy link
Member Author

fs-add-person has a unique situation where there is a dynamic number of requests that may be generated so we can't easily have them all in the DOM markup -- we must create and manage the requests all in JS.

I could have those requests add themselves as children to the fs-add-person element and then remove them after the response is received. I prefer to keep that logic in one place which lends towards the idea of a method on request elements, like delegateEvents() described above. In other words, we either have two possible implementations of delegateEvents():

  • Fire events on the specified element
  • Attach the request to the specified element

Firing events on the element would change the event target which at best is inconsistent and at worst it could be problematic.

Attaching the request as a child of the element could also create problems if the host element isn't aware that the request element exists (i.e. accidentally modify or remove the request). We might rely on developers to know the behavior of delegateEvents() and account for the request element being there but with a name like delegateEvents() the implementation won't be very obvious.

@justincy
Copy link
Member Author

Turns out we can't actually delegate events. Currently the only way to receive a response is by attaching an event listener to the response events so delegating events to another element breaks that contract. Therefore the only option we have left is to see that the elements are added to the DOM when we want events to propagate.

@justincy
Copy link
Member Author

I just realized that we don't need another event just for HTTP errors. After making all request events bubble (propagate) we can now just listen for response events and examine the response ourselves to determine whether there was an error.

justincy added a commit that referenced this issue Nov 30, 2017
@justincy
Copy link
Member Author

I have toast notifications appearing but the error messages are not particularly helpful. The API isn't very consistent about providing helpful error messages so the best I can do is Error: ${response.statusCode} ${repsonse.statusText}. In fs-webcomponents/fs-elements#2 we will figure out a more advanced error handling strategy.

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

No branches or pull requests

1 participant