-
-
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
feat(v2): enable feeds by default in blog plugin #3842
Changes from 1 commit
38af5f4
4f3199a
7f18351
9d2fe88
612687f
7ce4fec
0e25e50
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,7 +47,10 @@ test('should accept valid user options', async () => { | |
], | ||
}; | ||
const {value, error} = await PluginOptionSchema.validate(userOptions); | ||
expect(value).toEqual(userOptions); | ||
expect(value).toEqual({ | ||
...userOptions, | ||
feedOptions: {type: ['rss', 'atom']}, | ||
}); | ||
expect(error).toBe(undefined); | ||
}); | ||
|
||
|
@@ -82,6 +85,26 @@ test('should convert all feed type to array with other feed type', () => { | |
}); | ||
}); | ||
|
||
test('should contain array with rss + atom for missing feed type', () => { | ||
const {value} = PluginOptionSchema.validate({ | ||
feedOptions: {}, | ||
}); | ||
expect(value).toEqual({ | ||
...DEFAULT_OPTIONS, | ||
feedOptions: {type: ['rss', 'atom']}, | ||
}); | ||
}); | ||
|
||
test('should have array with rss + atom, title for missing feed type', () => { | ||
const {value} = PluginOptionSchema.validate({ | ||
feedOptions: {title: 'title'}, | ||
}); | ||
expect(value).toEqual({ | ||
...DEFAULT_OPTIONS, | ||
feedOptions: {type: ['rss', 'atom'], title: 'title'}, | ||
}); | ||
}); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That looks good Can you please add a test to ensure that type: null leads to type: null after validation? To ensure it's not using default values as a fallback? |
||
describe('blog sidebar', () => { | ||
test('should accept 0 sidebar count', () => { | ||
const userOptions = {blogSidebarCount: 0}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,12 +76,13 @@ export const PluginOptionSchema = Joi.object({ | |
DEFAULT_OPTIONS.beforeDefaultRehypePlugins, | ||
), | ||
feedOptions: Joi.object({ | ||
type: Joi.alternatives().conditional( | ||
Joi.string().equal('all', 'rss', 'atom'), | ||
{ | ||
type: Joi.alternatives() | ||
.conditional(Joi.string().equal('all', 'rss', 'atom'), { | ||
then: Joi.custom((val) => (val === 'all' ? ['rss', 'atom'] : [val])), | ||
}, | ||
), | ||
}) | ||
.empty(undefined) | ||
.empty(null) | ||
.default(['rss', 'atom']), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi, I've tried setting Also, setting Do you have any suggestions as to how to resolve these? The only way I've found to make it work is by adding the empty() and default() methods within the Joi object. Thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't debug remotely code that I can't fully see, You'd rather commit your failed attempt and ask a new review.
You should make the validator accept an array for this to work, currently it only accept strings There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok yeah, I'll make another commit with the failed attempt. |
||
title: Joi.string().allow(''), | ||
description: Joi.string().allow(''), | ||
copyright: Joi.string(), | ||
|
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.
Shouldn't this be added to
DEFAULT_OPTIONS
oruserOptions
?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.
yes
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.
I believe I tried what you suggested when trying to resolve the issue with this unit test. However, it won't pass the test unless we add
feedOptions: {type: ['rss', 'atom']}
hereThere 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.
why exactly this test does not pass?
I can't tell you if I can't see the code, but what I'm sure of is that we 100% want to put default options in the
DEFAULT_OPTIONS
constantThere 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.
I've now added
type: 'all'
intoDEFAULT_OPTIONS.feedOptions
and this test passes. I think it would still make sense to have this line of code here since it would converttype: 'all'
into the array (['rss', 'atom']
) right?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.
@cindygu4 I'd rather have
type: ['rss', 'atom']
and allow the validation to work with arrays syntax, so that the default options are not "transformed"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.
Thanks for the suggestion @slorber! @vikhramt007 and I have implemented the feature using the
type: ['rss', 'atom']
as the default and allowed the validation to work with arrays. It now passes all of the unit tests :)