-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
perf(v2): convert synchronous filewrite to asynchronous #2936
Conversation
…generate This looks like should return a Promise list , other than a sync io operation
Deploy preview for docusaurus-2 ready! Built with commit b7df851 |
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, that looks good to me
For consistency I'd prefer we move to async/await everywhere
try { | ||
fs.writeFileSync(feedPath, feedContent); | ||
} catch (err) { | ||
return fs.outputFile(feedPath, feedContent).catch((err) => { |
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.
Hi
What about using async/await here too. I think we should progressively update to async/await everywhere for consistency
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.
Hi @slorber ,
I know about that. But this a Promise.all
, and should return promise. I dont think async/await
is suitable.
async/await
is good syntax but not fit any case
If you still persist in it, i will update it.
Thanks for you review
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 is the problem with Promise.all and async/await?
We already have this in other places already
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.
like in packages/docusaurus-plugin-content-docs/src/index.ts:464
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.
Well, just think this too redundancy —— its unnecessary to write async/await
.
But if that is a project rule, i will comply with it.
for consistency
thanks |
my pleasure |
Motivation
I encounter same problem like #2539 in alpha.50 and discover its have been resolved. But the solution to fix is sync way.
Have you read the Contributing Guidelines on pull requests?
Yep
Test Plan
Build website and make sure that the
build/blog/atom.xml
,build/blog/rss.xml
andbuild/sitemap.xml
files are still work correct.Related PRs
(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)