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

V13 pre refactor #1624

Merged
merged 1 commit into from
Sep 28, 2022
Merged

V13 pre refactor #1624

merged 1 commit into from
Sep 28, 2022

Conversation

BenSurgisonGDS
Copy link
Contributor

@BenSurgisonGDS BenSurgisonGDS commented Sep 26, 2022

Prepare V13 for a refactor to allow plugins/extensions to be added without restarting the application:

  • Create a sessionUtilities from utils for session data to prevent a circular reference when refactoring later.
  • Add support for globals.
  • Catch and handle errors in server and filter api.
  • Fix the api tests so the fake router is mocked correctly.
  • Fix the numbering in the release docs.

@BenSurgisonGDS BenSurgisonGDS changed the base branch from main to v13 September 26, 2022 17:41
@BenSurgisonGDS BenSurgisonGDS marked this pull request as ready for review September 27, 2022 07:17
@BenSurgisonGDS BenSurgisonGDS force-pushed the v13-pre-refactor branch 2 times, most recently from b671132 to f43a0b1 Compare September 27, 2022 15:25
try {
return fse.readJsonSync(appConfigPath)
} catch (e) {
console.error(`Could not load config from ${appConfigPath}, please check your JSON is well formed.`)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how this error message will read to someone who isn't a developer, @NoraGDS what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you be happier if this said something like "Could not load config from ${appConfigPath}, please check your configuration is formatted correctly."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or would you prefer I revert the catch here also?

Copy link
Contributor

Choose a reason for hiding this comment

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

@lfdebrux doesn't ready too well. Can we say 'Check your configuration is formatted correctly in (whatever place it's in)'

lib/filters/api.js Show resolved Hide resolved
lib/utils.js Outdated Show resolved Hide resolved
lib/utils.js Outdated Show resolved Hide resolved
server.js Outdated Show resolved Hide resolved
@BenSurgisonGDS BenSurgisonGDS force-pushed the v13-pre-refactor branch 2 times, most recently from cbd118e to bfea15b Compare September 28, 2022 10:29
5. Update the [CHANGELOG.md](/CHANGELOG.md) by:
6. Update the [CHANGELOG.md](/CHANGELOG.md) by:
Copy link
Member

Choose a reason for hiding this comment

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

👍

@BenSurgisonGDS BenSurgisonGDS merged commit 06aad0c into v13 Sep 28, 2022
@BenSurgisonGDS BenSurgisonGDS deleted the v13-pre-refactor branch September 28, 2022 12:20
@lfdebrux lfdebrux mentioned this pull request Nov 17, 2022
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

Successfully merging this pull request may close these issues.

3 participants