-
Notifications
You must be signed in to change notification settings - Fork 4
Conversation
Yes, fix them in a separate commit from |
8d470b4
to
969c071
Compare
Here are the remaining style errors. I'm not too familiar with promises and regex. Also for use of reduce to create a set Lines 137 to 140 in ed21d5f
|
Line 154 in 969c071
Yes, the escape character is useless.
A |
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.
Pushed my proposed fixes: 9cac60a
lib/Site.js
Outdated
.filter(x => x.endsWith('site.json')) | ||
.map(x => path.resolve(x)); | ||
|
||
if (!candidates.length) return reject(); |
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.
@Gisonrg What do you think about this?
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 don't think we need to reject this if the candidates is empty. If we early return here then this.baseUrlMap
would be an undefined value.
It does not give us much performance improvement. If the candidates is empty, the following lookUp
will not run for the empty array.
If we concern about there could be no site.json
, if the site.json
in root is missing then readSiteConfig()
will complain about it. So here we should have at least one site.json
in the candidate.
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.
So 'reject' is defined but never used
should be ignored?
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.
If we just want to avoid the unused reject
, collectBaseUrl
is actually a sync function. No async operation involved. So instead of creating a new Promise, we could refactor it as:
Site.prototype.collectBaseUrl = function () {
const candidates
= walkSync(this.rootPath, { directories: false })
.filter(x => x.endsWith('site.json'))
.map(x => path.resolve(x));
this.baseUrlMap = candidates.reduce((pre, now) => {
// eslint-disable-next-line no-param-reassign
pre[path.dirname(now)] = true;
return pre;
}, {});
return Promise.resolve();
};
I think it should work as expected.
lib/Site.js
Outdated
pre[path.dirname(now)] = true | ||
const candidates | ||
= walkSync(this.rootPath, { directories: false }) | ||
.filter(x => x.endsWith('site.json')) |
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.
We can replace 'site.json'
with SITE_CONFIG_NAME
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.
Before we merge large refactoring changes like this, make sure it builds and could render site correctly.
Looks good. If it is finished please merge it |
@nicholaschuayunzhi Remove [WIP] when you're satisfied with your PR. |
@nicholaschuayunzhi Thanks for your contribution! |
Applied eslint --fix to entire project. There are still style errors. Shall I manually fix them? @acjh