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

Do not swallow errors from session-data-defaults.js #1050

Merged
merged 2 commits into from
Aug 24, 2021

Conversation

fofr
Copy link
Contributor

@fofr fofr commented Jul 30, 2021

If the code generating session-data-defaults is fairly complex, spotting and fixing those errors is a common problem, and is more than fixing simple syntax issues.

This change still allows the app to run, but will show the original error in the console alongside the warning about loading the defaults. By showing the full error it also highlights where there's a problem – the single line "Could not load…" message can be easily missed in a busy log.

Showing the error in the log allows a developer to debug and fix the problem.

@joelanman
Copy link
Contributor

thanks! Would you mind pasting a sample terminal output when there's an error?

@fofr
Copy link
Contributor Author

fofr commented Aug 4, 2021

@joelanman Sure, it's a standard stack-trace, so looks like this (I threw in a random error – I gave an object a trailing comma it should not have – and I can see that it's on line 14 of session-data-defaults).

Console before

[10:17:40] [nodemon] starting `node listen-on-port.js`
Could not load the session data defaults from app/data/session-data-defaults.js. Might be a syntax error?

GOV.UK Prototype Kit v9.3.0

NOTICE: the kit is for building prototypes, do not use it for production services.

Listening on port 3003   url: http://localhost:3003

Console after

[10:17:40] [nodemon] starting `node listen-on-port.js`
Could not load the session data defaults from app/data/session-data-defaults.js.
/app/data/session-data-defaults.js:14
const sessionCases = [
^^^^^

SyntaxError: Unexpected token 'const'
    at wrapSafe (internal/modules/cjs/loader.js:1053:16)
    at Module._compile (internal/modules/cjs/loader.js:1101:27)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1157:10)
    at Module.load (internal/modules/cjs/loader.js:985:32)
    at Function.Module._load (internal/modules/cjs/loader.js:878:14)
    at Module.require (internal/modules/cjs/loader.js:1025:19)
    at require (internal/modules/cjs/helpers.js:72:18)
    at Object.<anonymous> (lib/utils.js:270:25)
    at Module._compile (internal/modules/cjs/loader.js:1137:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1157:10)

GOV.UK Prototype Kit v9.3.0

NOTICE: the kit is for building prototypes, do not use it for production services.

Listening on port 3003   url: http://localhost:3003

@edwardhorsford
Copy link
Contributor

I think this solves #793

@edwardhorsford
Copy link
Contributor

Any reason not to prevent the app running? That might be more obvious that something has gone wrong.

@joelanman
Copy link
Contributor

maybe a newline \n between the message and the error might be a small thing to help with readability

@fofr
Copy link
Contributor Author

fofr commented Aug 4, 2021

Any reason not to prevent the app running? That might be more obvious that something has gone wrong.

I didn't want to change existing behaviour

@fofr
Copy link
Contributor Author

fofr commented Aug 4, 2021

@joelanman I didn't add a newline as the two things relate to each other, whereas elsewhere in the log the new lines distinguish separate things (like the kit version and the port being used)

@36degrees 36degrees requested a review from lfdebrux August 13, 2021 09:05
Copy link
Member

@lfdebrux lfdebrux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

@joelanman
Copy link
Contributor

in that case we just need to add a line for this in the changelog - are you ok to add that @fofr ?

@36degrees 36degrees added this to the [NEXT] milestone Aug 19, 2021
fofr added 2 commits August 20, 2021 20:09
If the code generating session-data-defaults is fairly complex, spotting and fixing those errors is a common problem, and is more than fixing simple syntax issues.

This change still allows the app to run, but will show the original error in the console alongside the warning about loading the defaults. By showing the full error it also highlights where there's a problem – the single line "Could not load" message can be easily missed in a busy log.

Showing the error in the log allows a developer to debug and fix the problem.
@fofr fofr force-pushed the error-session-default branch from 766b7d8 to 5fce377 Compare August 20, 2021 19:11
@fofr
Copy link
Contributor Author

fofr commented Aug 20, 2021

👍 @joelanman I've added a changelog entry

Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @fofr 👍🏻

@36degrees 36degrees merged commit 0424fd2 into alphagov:main Aug 24, 2021
@fofr fofr deleted the error-session-default branch August 25, 2021 10:54
@EoinShaughnessy EoinShaughnessy changed the title Don’t swallow errors from session-data-defaults.js Do not swallow errors from session-data-defaults.js Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

5 participants