-
Notifications
You must be signed in to change notification settings - Fork 123
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
Implement live preview fully for site.json #1529
Conversation
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.
Neat code! Would it be possible to add some tests for this?
…eload of site.json
packages/core/src/Site/index.js
Outdated
const { theme } = this.siteConfig; | ||
if (!theme || !_.has(SUPPORTED_THEMES_PATHS, theme)) { | ||
if (!toCopyDefault && (!theme || !_.has(SUPPORTED_THEMES_PATHS, theme))) { |
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.
Minor nit: would it be possible to remove the need for the toCopyDefault
? It seems like if toCopyDefault
is True and theme
is defined, then it would copy the theme's file instead of the default file.
What do you think of the following flow:
If theme
is undefined, then we will assume that the caller wants to copy the default bootstrap theme.
Else check if theme
is a supported theme and copy over the relevant theme file.
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 do you think of the following flow:
If theme is undefined, then we will assume that the caller wants to copy the default bootstrap theme.
Else check if theme is a supported theme and copy over the relevant theme file.
Hmm I think this will make a redundant copy of the default theme if no theme is specified in the initial build. Maybe I can rename toCopyDefault
to isRebuild
so that it is more concise. What do you think?
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.
Hmm I think this will make a redundant copy of the default theme if no theme is specified in the initial build.
Hmm, I see. In that case, should we log a warning / throw exception for the case toCopyDefault = True && theme
?
Maybe I can rename toCopyDefault to isRebuild so that it is more concise. What do you think?
I am fine with either 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.
Sorry I think I wasn't too clear about this 😅. I think I should rename toCopyDefault
as it is a little misleading. This parameter should only be True if it is a rebuild (site.json was modified).
What do you think about this check:
if ((!isRebuild && !theme) || (theme && !_.has(SUPPORTED_THEMES_PATHS, theme))) {
return _.noop;
}
It will be a noop
during the first build if theme is not specified/theme is invalid. Else, if it is subsequent rebuild/theme is specified and valid, then, it will copy over the relevant theme's file (including default theme if !theme
).
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 that looks good! Can you also add a comment next to this check to indicate its purpose for future reference?
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.
Alright I've updated this and also added the comment :)
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! @jonahtanjz
Since most of the attributes in site.json (except baseUrl, pages and deploy) will affect majority of the pages, hence all pages will be regenerated when any of these attributes (except those listed above) is modified.
Hmm shouldn't baseUrl
be included? ({{ baseUrl }}
variable changes).
Although, there might not be much value in supporting this in serve
mode (since baseUrl
would really only change for deployment), we can document this exception if so.
Plugins would need some special handling as well like themes
and ignore
. (see PluginManager
)
(optional) You can also consider implementing the fourth part of #1495 first as well to ensure all pages have the same updated copy of SiteConfig
's properties
packages/core/src/Site/index.js
Outdated
|| !_.isEqual(oldSiteConfig.style, this.siteConfig.style) | ||
|| !_.isEqual(oldSiteConfig.externalScripts, this.siteConfig.externalScripts) | ||
|| !_.isEqual(oldSiteConfig.globalOverride, this.siteConfig.globalOverride) | ||
|| !_.isEqual(oldSiteConfig.ignore, this.siteConfig.ignore) |
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's this for?
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 to check if any of these configs have changed and if they have changed, would require a rebuild of all pages.
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.
(ignore
in particular) Shouldn't be necessary I think; ignore
is only used down the line for link validation warnings
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.
Ahh ok. I was under the impression that if a page is dependent on an asset that is later added to ignore
, then that entire page will have to be rebuilt. Might have misunderstood the functionality of ignore
here 😅
@ang-zeyu Thanks for review :)
Since {{ baseUrl }} is a variable change then I supposed it would make sense to be included so that the behaviour would be consistent.
For this portion were you referring to running |
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.
Since {{ baseUrl }} is a variable change then I supposed it would make sense to be included so that the behaviour would be consistent.
@jonahtanjz its common and ok to forgo supporting something if the use case is practically zero, especially since baseUrl
needs extra special care.
I might be missing a use case though. Could get a user opinion from @damithc here. (would live-reload for baseUrl
parameter in site config be useful?)
For this portion were you referring to running collectBaseUrl() method? Since this is the only part where a new PluginManager is created and then subsequently assigned to all the new pages that will be rebuilt.
Was thinking a more targeted approach that dosen't rerun the baseUrl collection. (can do a simple refactor / extraction of method here)
On that note (good catch), External/LayoutManager
's copies in collectBaseUrl()
should be refreshed as well.
packages/core/src/Site/index.js
Outdated
|| !_.isEqual(oldSiteConfig.style, this.siteConfig.style) | ||
|| !_.isEqual(oldSiteConfig.externalScripts, this.siteConfig.externalScripts) | ||
|| !_.isEqual(oldSiteConfig.globalOverride, this.siteConfig.globalOverride) | ||
|| !_.isEqual(oldSiteConfig.ignore, this.siteConfig.ignore) |
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.
(ignore
in particular) Shouldn't be necessary I think; ignore
is only used down the line for link validation warnings
I think it is OK not support live preview support for changes to the baseUrl as it is a very rare case. Just need to specify it clearly. |
@ang-zeyu @raysonkoh I've updated the PR to include the exception of not supporting live reload for |
packages/core/src/Site/index.js
Outdated
|| !_.isEqual(oldSiteConfig.plugins, this.siteConfig.plugins) | ||
|| !_.isEqual(oldSiteConfig.pluginsContext, this.siteConfig.pluginsContext) | ||
|| !_.isEqual(oldSiteConfig.disableHtmlBeautify, this.siteConfig.disableHtmlBeautify) | ||
|| !_.isEqual(oldSiteConfig.intrasiteLinkValidation, this.siteConfig.intrasiteLinkValidation)) { |
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.
missing timeZone / locale / style
as well - should / could we just remove the checks here as well?
I feel there's not much value in further narrowing it down since buildManagers
isn't very costly
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 feel there's not much value in further narrowing it down since buildManagers isn't very costly
Alright since there's not much value for the checks as it is not costly, then buildManagers
will be called for every full rebuild and whenever ignore
is modified. I've updated the PR accordingly.
@ang-zeyu Thanks for the catch and suggestions. I've updated the PR to remove the further checks for |
docs/userGuide/siteJsonFile.md
Outdated
@@ -81,6 +81,10 @@ Here is a typical `site.json` file: | |||
|
|||
<include src="deployingTheSite.md#warning-about-baseUrl" /> | |||
|
|||
<box type="warning"> | |||
|
|||
Note: `baseUrl` does not support live reload as it should not change in `markbind serve` mode. |
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.
Let's update the definition here as well with this https://deploy-preview-1529--markbind-master.netlify.app/userguide/glossary
Minor wording nit:
Note: `baseUrl` does not support live reload as it should not change in `markbind serve` mode. | |
Note: `baseUrl` does not support live preview as there is no use case for changing it in during `markbind serve`. |
Otherwise, Lgtm! 👍
Let's wait and see if @raysonkoh has anything to add
await this.removeAsset(removedPages); | ||
this.buildManagers(); | ||
await this._rebuildSourceFiles(); |
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.
Additional remark (no action needed):
EDIT: After looking around at the other handlers (addHandler and removeHandler), it seems all pages should be rebuilt if the added/removed file is a page/dependency of page.
This has been existing behaviour for a long while already but can probably be made into an issue 🙂
Per #1514's comment, the number of dependent pages generated / regenerated can be improved later as well.
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.
Alright. I'll open an issue for this and the previous discussion.
@ang-zeyu @raysonkoh I've updated the PR accordingly :) |
docs/userGuide/glossary.md
Outdated
@@ -10,8 +10,6 @@ | |||
**_Live preview_** is: | |||
- Regeneration of affected content upon any change to <tooltip content="`.md`, `.mbd`, `.mbdf`, `.njk` files ... anything your content depends on!">source files</tooltip>, then reloading the updated site in the Browser. | |||
|
|||
- Regeneration will also occur upon any modification to attributes in `site.json` with the exception of [`baseUrl`](siteJsonFile.md#baseurl-no-live-preview). |
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.
👀
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've re-added this portion
LGTM 👍 |
What is the purpose of this pull request?
Overview of changes:
Resolves #48.
A follow up to #1514, which supported live preview for the pages attribute. This PR will support live preview for all the other attributes in
site.json
. A server restart is no longer required for these changes to be reflected when serving the site throughmarkbind serve
. Since most of the attributes insite.json
(exceptbaseUrl
,pages
anddeploy
) will affect majority of the pages, hence all pages will be regenerated when any of these attributes (except those listed above) is modified.For the
style
attribute, if abootswatch
theme is specified/added, itsbootstrap.min.css
theme file will overwrite the existing file and all pages are rebuilt. Similarly, if abootswatch
theme is removed, the defaultbootstrap.min.css
, which is located atcore-web/asset/css
will overwrite the existing file.Anything you'd like to highlight / discuss:
Would appreciate if you can let me know if I missed any attributes or if any of the attributes (other than those listed above) that does not require all pages to be rebuilt when it is modified.
Testing instructions:
markbind serve
Proposed commit message: (wrap lines at 72 characters)
Implement live preview for site.json
When attributes other than pages in site.json are modified, a server
restart is required to reflect the changes on the live preview. This can
become a hassle to the user as they will need to frequently restart
the server when trying out different site configuration.
Let's support live preview for all the attributes in site.json so that the
latest changes can be reflected without restarting the server.
Checklist: ☑️