Skip to content
This repository has been archived by the owner on Jul 12, 2024. It is now read-only.

Refactor for better logging #151

Merged
merged 7 commits into from
Aug 25, 2017
Merged

Refactor for better logging #151

merged 7 commits into from
Aug 25, 2017

Conversation

davidtheclark
Copy link
Contributor

This is going to turn out to be a pretty monstrous refactor PR. All the refactoring will aim to improve logging and error handling.

We'll definitely want to try out these changes on all the examples and some of our existing Batfish sites before releasing.

@davidtheclark
Copy link
Contributor Author

Well ... I believe this is working now. Lots and lots of refactoring. I'd checked against development and production builds of a bunch of the examples.

@jfurrow would you mind trying this out? We need to check two things:

  • Does Batfish still work the same?
  • Are errors getting reported in helpful ways? You could try errors like:
    • Invalid configuration of all kinds
    • Invalid CSS
    • Bad JS syntax
    • Using window in a place where it can't be used because of the static build
    • etc. Anything else you can think of, really. I'm trying to show helpful errors when it's an expected user error. (But if it's totally unexpected and really shouldn't happen, regular old errors).

@davidtheclark davidtheclark changed the title [WIP] Refactor for better logging Refactor for better logging Aug 24, 2017
@davidtheclark
Copy link
Contributor Author

That was a very challenging rebase.

driving-with-gopher

@jfurrow
Copy link
Contributor

jfurrow commented Aug 25, 2017

+💯 for improved error handling!

It doesn't display an error when providing an incorrect path to a stylesheet — should it? It does display a 404 when an external stylesheet doesn't exist.

@davidtheclark
Copy link
Contributor Author

It doesn't display an error when providing an incorrect path to a stylesheet

Aha, that does remind me that I should add more checks for whether files and directories exist. Not just stylesheets, but also pagesDirectory, applicationWrapperPath, etc.

@davidtheclark
Copy link
Contributor Author

@jfurrow: I think I have some decent checks in place now, for whether your applicationWrapperPath exists, your pagesDirectory exists, and the things in your stylesheets array are valid. Could you please check that again, see if it works intuitively?

Took longer then expected. I need to go wash some dishes!

@jfurrow
Copy link
Contributor

jfurrow commented Aug 25, 2017

@davidtheclark That's great, definitely works intuitively.

Building the optimizing-loaders example fails in this branch, but working on master.
screen shot 2017-08-24 at 10 30 28 pm

@jfurrow
Copy link
Contributor

jfurrow commented Aug 25, 2017

Everything else seems to be working great!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants