-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
chore(v2): use joi for config validation #2987
chore(v2): use joi for config validation #2987
Conversation
Deploy preview for docusaurus-2 ready! Built with commit 16f4ea0 |
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.
That looks good
Would just factorize a bit the tests, and test for empty config
baseUrl: '/', | ||
favicon: 'some.ico', | ||
title: 'my site', | ||
url: 'https://mysite.com', |
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.
What about extracting this in a "baseConfig" variable?
Maybe it could be more convenient to have such method to make tests less "verbose"
const testConfig = (config) => Joi.attempt({...baseConfig,...config})
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.
Good idea , I will fix it.
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.
see comments
import {ConfigSchema, DEFAULT_CONFIG} from '../configValidation'; | ||
|
||
const testConfig = (config) => | ||
Joi.attempt({...DEFAULT_CONFIG, ...config}, ConfigSchema); |
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.
This is not really what I meant :p
The idea is that you don't need to mention the 4 required attributes in every test to avoid unnecessary boilerplate.
There are many ways to do here, here's an idea:
const baseTestConfig = {
baseUrl: "/",
favicon: "some.ico",
title: "my site",
url: "https://mysite.com"
};
const testConfig = config =>
Joi.attempt({...baseTestConfig,...config}, ConfigSchema);
const expectConfig = config => expect(config).toEqual({
...DEFAULT_CONFIG,
...baseTestConfig,
...config,
});
test('custom fields', () => {
const value = testConfig({
customFields: {
author: 'anshul',
},
});
expectConfig({
customFields: {
author: 'anshul',
},
});
}
} | ||
|
||
return config; | ||
return Joi.attempt(loadedConfig, ConfigSchema); |
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.
here it would be better to abortEarly: false
so you need to catch the potential validationError, extract all the errors, and rethrow as a well-formatted regular error (with line breaks etc...)
packages/docusaurus/src/server/__tests__/__snapshots__/configValidation.test.ts.snap
Outdated
Show resolved
Hide resolved
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.
LGTM
Motivation
More robust validation for config. Validating more fields and types. It fixes #2978.
Have you read the Contributing Guidelines on pull requests?
yes
Test Plan
I have added tests for validation and update old ones