Skip to content

Update post-checkout hook to auto build backend - #1994

Merged
tlylt merged 4 commits into
MarkBind:masterfrom
tlylt:update-hooks
Aug 10, 2022
Merged

Update post-checkout hook to auto build backend#1994
tlylt merged 4 commits into
MarkBind:masterfrom
tlylt:update-hooks

Conversation

@tlylt

@tlylttlylt commented Aug 7, 2022

Copy link
Copy Markdown
Contributor

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
Related to discussions in #1934

Anything you'd like to highlight/discuss:
Post checkout hook inputs reference: https://www.atlassian.com/git/tutorials/git-hooks#:~:text=your%20continuous%20integration.-,Post%2DCheckout,-The

Testing instructions:

  • Checkout to this PR branch, the following should be shown
    image
  • Afterwards, can cd docs and markbind serve -d to see that the docs can be served
  • Try modifying the "README.md" file, and discard it via git commit -- README.md, the post checkout hook should not be triggered

Proposed commit message: (wrap lines at 72 characters)
Update post-checkout hook to auto build backend


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

@jonahtanjzjonahtanjz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tlylt 👍

Just one suggestion :)

Comment thread.githooks/post-checkout Outdated
@tlylt
tlylt requested a review from ong6August 9, 2022 09:25

@jonahtanjzjonahtanjz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@jonahtanjzjonahtanjz added this to the 4.0.1 milestone Aug 9, 2022
@tlylt
tlylt merged commit 4912410 into MarkBind:masterAug 10, 2022
@ang-zeyu

Copy link
Copy Markdown
Contributor

Not sure if I mentioned this in #1934 as well, but if not then take it as a new point. This tackles only a very small subset of cases if the intention is to streamline the development experience.

Collaborative ones:

  • Fresh repo clone
  • git pull
  • git merge master

Individual ones:

  • git stash pop
  • copy pasting file from some other place on your PC
  • ❔ git is powerful. there are probably a lot more cases where source files can change not related to a checkout

Again, I think we should be providing separate commands that does the build by default to account for all of these. I emphasize separate, so if you don't need to rebuild the typescript files, you don't need to run a watcher -- just build the files once, or not at all, we can let the developer decide based on their individual preferences, workflow (did you do a git stash? merge? ...), PC capabilities (referring the point on tsc watcher being expensive).

While post-checkout is handy, I do not believe we should be relying solely (or at all, for the sake of consistency ("magic" 🎁 that sometimes works but sometimes dosen't)) on it to produce the build artifacts (typescript / anything else) necessary for development. The same goes for cleaning up artifacts in clean.jsvia post-checkout (I only realised this one in retrospect), which does not account for git pull / merge / ....

It also triggers many unnecessary rebuilds / cleans (e.g. checkout to another branch whose HEAD is referencing the same commit).

tldr I think this is ok as an interim solution, I'm still favouring supporting a few separate nuke-everything commands to streamline the development:

// can be less verbose
// the separate commands for the templates can probably be made into cli args as well.
"buildAndDevDocs": "npm run clean && npm run build:backend && node ...index.js s -d -o thedocs",
"buildAndDevDefaultTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o defaulttemplate",
"buildAndDevMinimalTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o minimaltemplate",
// can be less verbose
"watchAndDevDocs": "npm run clean && run the tsc watcher ... && node ...index.js s -d -o thedoc",
"watchAndDevDefaultTemplate": "...",
"watchAndDevMinimalTemplate": "...",
...

@tlylt

Copy link
Copy Markdown
ContributorAuthor

Thanks @ang-zeyu for the detailed comment. Think I should have mentioned more clearly that this is indeed an interim fix in the hope of improving the developer experience.

While not capturing all the cases, especially the unnecessary rebuild when checking out to a new branch whose HEAD is referencing the same commit, it solves most of the issues I faced in the past few weeks of somewhat active development and code review here. For some of the cases listed, they are certainly not in the purview of this PR which makes use of "post-checkout" (and not pull, or merge hooks) that hopefully addresses the case of.......checkout. On the point of "magic", I was thinking that the CLI output of the githook gives quite a bit of hint as to what commands are executed, so it alludes to the underlying working of the script. Of course, this does not replace the details in our DG.

Again, I think we should be providing separate commands that does the build by default to account for all of these. I emphasize separate, so if you don't need to rebuild the typescript files, you don't need to run a watcher -- just build the files once, or not at all, we can let the developer decide based on their individual preferences, workflow (did you do a git stash? merge? ...), PC capabilities (referring the point on tsc watcher being expensive).

tldr I think this is ok as an interim solution, I'm still favouring supporting a few separate nuke-everything commands to streamline the development:

// can be less verbose
// the separate commands for the templates can probably be made into cli args as well.
"buildAndDevDocs": "npm run clean && npm run build:backend && node ...index.js s -d -o thedocs",
"buildAndDevDefaultTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o defaulttemplate",
"buildAndDevMinimalTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o minimaltemplate",
// can be less verbose
"watchAndDevDocs": "npm run clean && run the tsc watcher ... && node ...index.js s -d -o thedoc",
"watchAndDevDefaultTemplate": "...",
"watchAndDevMinimalTemplate": "...",
...

Good points here 👍 would defer to you to put up a tracking issue whenever you are free since I am not entirely sure of any additional details/requirements of the solution you have in mind

@ang-zeyu

Copy link
Copy Markdown
Contributor

🙆‍♂️

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tlylt@ang-zeyu@jonahtanjz
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Update post-checkout hook to auto build backend by tlylt · Pull Request #1994 · MarkBind/markbind · GitHub
Skip to content

Update post-checkout hook to auto build backend - #1994

Merged
tlylt merged 4 commits into
MarkBind:masterfrom
tlylt:update-hooks
Aug 10, 2022
Merged

Update post-checkout hook to auto build backend#1994
tlylt merged 4 commits into
MarkBind:masterfrom
tlylt:update-hooks

Conversation

@tlylt

@tlylttlylt commented Aug 7, 2022

Copy link
Copy Markdown
Contributor

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
Related to discussions in #1934

Anything you'd like to highlight/discuss:
Post checkout hook inputs reference: https://www.atlassian.com/git/tutorials/git-hooks#:~:text=your%20continuous%20integration.-,Post%2DCheckout,-The

Testing instructions:

  • Checkout to this PR branch, the following should be shown
    image
  • Afterwards, can cd docs and markbind serve -d to see that the docs can be served
  • Try modifying the "README.md" file, and discard it via git commit -- README.md, the post checkout hook should not be triggered

Proposed commit message: (wrap lines at 72 characters)
Update post-checkout hook to auto build backend


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

@jonahtanjzjonahtanjz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tlylt 👍

Just one suggestion :)

Comment thread.githooks/post-checkout Outdated
@tlylt
tlylt requested a review from ong6August 9, 2022 09:25

@jonahtanjzjonahtanjz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@jonahtanjzjonahtanjz added this to the 4.0.1 milestone Aug 9, 2022
@tlylt
tlylt merged commit 4912410 into MarkBind:masterAug 10, 2022
@ang-zeyu

Copy link
Copy Markdown
Contributor

Not sure if I mentioned this in #1934 as well, but if not then take it as a new point. This tackles only a very small subset of cases if the intention is to streamline the development experience.

Collaborative ones:

  • Fresh repo clone
  • git pull
  • git merge master

Individual ones:

  • git stash pop
  • copy pasting file from some other place on your PC
  • ❔ git is powerful. there are probably a lot more cases where source files can change not related to a checkout

Again, I think we should be providing separate commands that does the build by default to account for all of these. I emphasize separate, so if you don't need to rebuild the typescript files, you don't need to run a watcher -- just build the files once, or not at all, we can let the developer decide based on their individual preferences, workflow (did you do a git stash? merge? ...), PC capabilities (referring the point on tsc watcher being expensive).

While post-checkout is handy, I do not believe we should be relying solely (or at all, for the sake of consistency ("magic" 🎁 that sometimes works but sometimes dosen't)) on it to produce the build artifacts (typescript / anything else) necessary for development. The same goes for cleaning up artifacts in clean.jsvia post-checkout (I only realised this one in retrospect), which does not account for git pull / merge / ....

It also triggers many unnecessary rebuilds / cleans (e.g. checkout to another branch whose HEAD is referencing the same commit).

tldr I think this is ok as an interim solution, I'm still favouring supporting a few separate nuke-everything commands to streamline the development:

// can be less verbose
// the separate commands for the templates can probably be made into cli args as well.
"buildAndDevDocs": "npm run clean && npm run build:backend && node ...index.js s -d -o thedocs",
"buildAndDevDefaultTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o defaulttemplate",
"buildAndDevMinimalTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o minimaltemplate",
// can be less verbose
"watchAndDevDocs": "npm run clean && run the tsc watcher ... && node ...index.js s -d -o thedoc",
"watchAndDevDefaultTemplate": "...",
"watchAndDevMinimalTemplate": "...",
...

@tlylt

Copy link
Copy Markdown
ContributorAuthor

Thanks @ang-zeyu for the detailed comment. Think I should have mentioned more clearly that this is indeed an interim fix in the hope of improving the developer experience.

While not capturing all the cases, especially the unnecessary rebuild when checking out to a new branch whose HEAD is referencing the same commit, it solves most of the issues I faced in the past few weeks of somewhat active development and code review here. For some of the cases listed, they are certainly not in the purview of this PR which makes use of "post-checkout" (and not pull, or merge hooks) that hopefully addresses the case of.......checkout. On the point of "magic", I was thinking that the CLI output of the githook gives quite a bit of hint as to what commands are executed, so it alludes to the underlying working of the script. Of course, this does not replace the details in our DG.

Again, I think we should be providing separate commands that does the build by default to account for all of these. I emphasize separate, so if you don't need to rebuild the typescript files, you don't need to run a watcher -- just build the files once, or not at all, we can let the developer decide based on their individual preferences, workflow (did you do a git stash? merge? ...), PC capabilities (referring the point on tsc watcher being expensive).

tldr I think this is ok as an interim solution, I'm still favouring supporting a few separate nuke-everything commands to streamline the development:

// can be less verbose
// the separate commands for the templates can probably be made into cli args as well.
"buildAndDevDocs": "npm run clean && npm run build:backend && node ...index.js s -d -o thedocs",
"buildAndDevDefaultTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o defaulttemplate",
"buildAndDevMinimalTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o minimaltemplate",
// can be less verbose
"watchAndDevDocs": "npm run clean && run the tsc watcher ... && node ...index.js s -d -o thedoc",
"watchAndDevDefaultTemplate": "...",
"watchAndDevMinimalTemplate": "...",
...

Good points here 👍 would defer to you to put up a tracking issue whenever you are free since I am not entirely sure of any additional details/requirements of the solution you have in mind

@ang-zeyu

Copy link
Copy Markdown
Contributor

🙆‍♂️

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tlylt@ang-zeyu@jonahtanjz
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Update post-checkout hook to auto build backend by tlylt · Pull Request #1994 · MarkBind/markbind · GitHub
Skip to content

Update post-checkout hook to auto build backend - #1994

Merged
tlylt merged 4 commits into
MarkBind:masterfrom
tlylt:update-hooks
Aug 10, 2022
Merged

Update post-checkout hook to auto build backend#1994
tlylt merged 4 commits into
MarkBind:masterfrom
tlylt:update-hooks

Conversation

@tlylt

@tlylttlylt commented Aug 7, 2022

Copy link
Copy Markdown
Contributor

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
Related to discussions in #1934

Anything you'd like to highlight/discuss:
Post checkout hook inputs reference: https://www.atlassian.com/git/tutorials/git-hooks#:~:text=your%20continuous%20integration.-,Post%2DCheckout,-The

Testing instructions:

  • Checkout to this PR branch, the following should be shown
    image
  • Afterwards, can cd docs and markbind serve -d to see that the docs can be served
  • Try modifying the "README.md" file, and discard it via git commit -- README.md, the post checkout hook should not be triggered

Proposed commit message: (wrap lines at 72 characters)
Update post-checkout hook to auto build backend


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

@jonahtanjzjonahtanjz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tlylt 👍

Just one suggestion :)

Comment thread.githooks/post-checkout Outdated
@tlylt
tlylt requested a review from ong6August 9, 2022 09:25

@jonahtanjzjonahtanjz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@jonahtanjzjonahtanjz added this to the 4.0.1 milestone Aug 9, 2022
@tlylt
tlylt merged commit 4912410 into MarkBind:masterAug 10, 2022
@ang-zeyu

Copy link
Copy Markdown
Contributor

Not sure if I mentioned this in #1934 as well, but if not then take it as a new point. This tackles only a very small subset of cases if the intention is to streamline the development experience.

Collaborative ones:

  • Fresh repo clone
  • git pull
  • git merge master

Individual ones:

  • git stash pop
  • copy pasting file from some other place on your PC
  • ❔ git is powerful. there are probably a lot more cases where source files can change not related to a checkout

Again, I think we should be providing separate commands that does the build by default to account for all of these. I emphasize separate, so if you don't need to rebuild the typescript files, you don't need to run a watcher -- just build the files once, or not at all, we can let the developer decide based on their individual preferences, workflow (did you do a git stash? merge? ...), PC capabilities (referring the point on tsc watcher being expensive).

While post-checkout is handy, I do not believe we should be relying solely (or at all, for the sake of consistency ("magic" 🎁 that sometimes works but sometimes dosen't)) on it to produce the build artifacts (typescript / anything else) necessary for development. The same goes for cleaning up artifacts in clean.jsvia post-checkout (I only realised this one in retrospect), which does not account for git pull / merge / ....

It also triggers many unnecessary rebuilds / cleans (e.g. checkout to another branch whose HEAD is referencing the same commit).

tldr I think this is ok as an interim solution, I'm still favouring supporting a few separate nuke-everything commands to streamline the development:

// can be less verbose
// the separate commands for the templates can probably be made into cli args as well.
"buildAndDevDocs": "npm run clean && npm run build:backend && node ...index.js s -d -o thedocs",
"buildAndDevDefaultTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o defaulttemplate",
"buildAndDevMinimalTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o minimaltemplate",
// can be less verbose
"watchAndDevDocs": "npm run clean && run the tsc watcher ... && node ...index.js s -d -o thedoc",
"watchAndDevDefaultTemplate": "...",
"watchAndDevMinimalTemplate": "...",
...

@tlylt

Copy link
Copy Markdown
ContributorAuthor

Thanks @ang-zeyu for the detailed comment. Think I should have mentioned more clearly that this is indeed an interim fix in the hope of improving the developer experience.

While not capturing all the cases, especially the unnecessary rebuild when checking out to a new branch whose HEAD is referencing the same commit, it solves most of the issues I faced in the past few weeks of somewhat active development and code review here. For some of the cases listed, they are certainly not in the purview of this PR which makes use of "post-checkout" (and not pull, or merge hooks) that hopefully addresses the case of.......checkout. On the point of "magic", I was thinking that the CLI output of the githook gives quite a bit of hint as to what commands are executed, so it alludes to the underlying working of the script. Of course, this does not replace the details in our DG.

Again, I think we should be providing separate commands that does the build by default to account for all of these. I emphasize separate, so if you don't need to rebuild the typescript files, you don't need to run a watcher -- just build the files once, or not at all, we can let the developer decide based on their individual preferences, workflow (did you do a git stash? merge? ...), PC capabilities (referring the point on tsc watcher being expensive).

tldr I think this is ok as an interim solution, I'm still favouring supporting a few separate nuke-everything commands to streamline the development:

// can be less verbose
// the separate commands for the templates can probably be made into cli args as well.
"buildAndDevDocs": "npm run clean && npm run build:backend && node ...index.js s -d -o thedocs",
"buildAndDevDefaultTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o defaulttemplate",
"buildAndDevMinimalTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o minimaltemplate",
// can be less verbose
"watchAndDevDocs": "npm run clean && run the tsc watcher ... && node ...index.js s -d -o thedoc",
"watchAndDevDefaultTemplate": "...",
"watchAndDevMinimalTemplate": "...",
...

Good points here 👍 would defer to you to put up a tracking issue whenever you are free since I am not entirely sure of any additional details/requirements of the solution you have in mind

@ang-zeyu

Copy link
Copy Markdown
Contributor

🙆‍♂️

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tlylt@ang-zeyu@jonahtanjz
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Update post-checkout hook to auto build backend by tlylt · Pull Request #1994 · MarkBind/markbind · GitHub
Skip to content

Update post-checkout hook to auto build backend - #1994

Merged
tlylt merged 4 commits into
MarkBind:masterfrom
tlylt:update-hooks
Aug 10, 2022
Merged

Update post-checkout hook to auto build backend#1994
tlylt merged 4 commits into
MarkBind:masterfrom
tlylt:update-hooks

Conversation

@tlylt

@tlylttlylt commented Aug 7, 2022

Copy link
Copy Markdown
Contributor

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
Related to discussions in #1934

Anything you'd like to highlight/discuss:
Post checkout hook inputs reference: https://www.atlassian.com/git/tutorials/git-hooks#:~:text=your%20continuous%20integration.-,Post%2DCheckout,-The

Testing instructions:

  • Checkout to this PR branch, the following should be shown
    image
  • Afterwards, can cd docs and markbind serve -d to see that the docs can be served
  • Try modifying the "README.md" file, and discard it via git commit -- README.md, the post checkout hook should not be triggered

Proposed commit message: (wrap lines at 72 characters)
Update post-checkout hook to auto build backend


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

@jonahtanjzjonahtanjz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tlylt 👍

Just one suggestion :)

Comment thread.githooks/post-checkout Outdated
@tlylt
tlylt requested a review from ong6August 9, 2022 09:25

@jonahtanjzjonahtanjz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@jonahtanjzjonahtanjz added this to the 4.0.1 milestone Aug 9, 2022
@tlylt
tlylt merged commit 4912410 into MarkBind:masterAug 10, 2022
@ang-zeyu

Copy link
Copy Markdown
Contributor

Not sure if I mentioned this in #1934 as well, but if not then take it as a new point. This tackles only a very small subset of cases if the intention is to streamline the development experience.

Collaborative ones:

  • Fresh repo clone
  • git pull
  • git merge master

Individual ones:

  • git stash pop
  • copy pasting file from some other place on your PC
  • ❔ git is powerful. there are probably a lot more cases where source files can change not related to a checkout

Again, I think we should be providing separate commands that does the build by default to account for all of these. I emphasize separate, so if you don't need to rebuild the typescript files, you don't need to run a watcher -- just build the files once, or not at all, we can let the developer decide based on their individual preferences, workflow (did you do a git stash? merge? ...), PC capabilities (referring the point on tsc watcher being expensive).

While post-checkout is handy, I do not believe we should be relying solely (or at all, for the sake of consistency ("magic" 🎁 that sometimes works but sometimes dosen't)) on it to produce the build artifacts (typescript / anything else) necessary for development. The same goes for cleaning up artifacts in clean.jsvia post-checkout (I only realised this one in retrospect), which does not account for git pull / merge / ....

It also triggers many unnecessary rebuilds / cleans (e.g. checkout to another branch whose HEAD is referencing the same commit).

tldr I think this is ok as an interim solution, I'm still favouring supporting a few separate nuke-everything commands to streamline the development:

// can be less verbose
// the separate commands for the templates can probably be made into cli args as well.
"buildAndDevDocs": "npm run clean && npm run build:backend && node ...index.js s -d -o thedocs",
"buildAndDevDefaultTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o defaulttemplate",
"buildAndDevMinimalTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o minimaltemplate",
// can be less verbose
"watchAndDevDocs": "npm run clean && run the tsc watcher ... && node ...index.js s -d -o thedoc",
"watchAndDevDefaultTemplate": "...",
"watchAndDevMinimalTemplate": "...",
...

@tlylt

Copy link
Copy Markdown
ContributorAuthor

Thanks @ang-zeyu for the detailed comment. Think I should have mentioned more clearly that this is indeed an interim fix in the hope of improving the developer experience.

While not capturing all the cases, especially the unnecessary rebuild when checking out to a new branch whose HEAD is referencing the same commit, it solves most of the issues I faced in the past few weeks of somewhat active development and code review here. For some of the cases listed, they are certainly not in the purview of this PR which makes use of "post-checkout" (and not pull, or merge hooks) that hopefully addresses the case of.......checkout. On the point of "magic", I was thinking that the CLI output of the githook gives quite a bit of hint as to what commands are executed, so it alludes to the underlying working of the script. Of course, this does not replace the details in our DG.

Again, I think we should be providing separate commands that does the build by default to account for all of these. I emphasize separate, so if you don't need to rebuild the typescript files, you don't need to run a watcher -- just build the files once, or not at all, we can let the developer decide based on their individual preferences, workflow (did you do a git stash? merge? ...), PC capabilities (referring the point on tsc watcher being expensive).

tldr I think this is ok as an interim solution, I'm still favouring supporting a few separate nuke-everything commands to streamline the development:

// can be less verbose
// the separate commands for the templates can probably be made into cli args as well.
"buildAndDevDocs": "npm run clean && npm run build:backend && node ...index.js s -d -o thedocs",
"buildAndDevDefaultTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o defaulttemplate",
"buildAndDevMinimalTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o minimaltemplate",
// can be less verbose
"watchAndDevDocs": "npm run clean && run the tsc watcher ... && node ...index.js s -d -o thedoc",
"watchAndDevDefaultTemplate": "...",
"watchAndDevMinimalTemplate": "...",
...

Good points here 👍 would defer to you to put up a tracking issue whenever you are free since I am not entirely sure of any additional details/requirements of the solution you have in mind

@ang-zeyu

Copy link
Copy Markdown
Contributor

🙆‍♂️

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tlylt@ang-zeyu@jonahtanjz
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' Update post-checkout hook to auto build backend by tlylt · Pull Request #1994 · MarkBind/markbind · GitHub
Skip to content

Update post-checkout hook to auto build backend - #1994

Merged
tlylt merged 4 commits into
MarkBind:masterfrom
tlylt:update-hooks
Aug 10, 2022
Merged

Update post-checkout hook to auto build backend#1994
tlylt merged 4 commits into
MarkBind:masterfrom
tlylt:update-hooks

Conversation

@tlylt

@tlylttlylt commented Aug 7, 2022

Copy link
Copy Markdown
Contributor

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
Related to discussions in #1934

Anything you'd like to highlight/discuss:
Post checkout hook inputs reference: https://www.atlassian.com/git/tutorials/git-hooks#:~:text=your%20continuous%20integration.-,Post%2DCheckout,-The

Testing instructions:

  • Checkout to this PR branch, the following should be shown
    image
  • Afterwards, can cd docs and markbind serve -d to see that the docs can be served
  • Try modifying the "README.md" file, and discard it via git commit -- README.md, the post checkout hook should not be triggered

Proposed commit message: (wrap lines at 72 characters)
Update post-checkout hook to auto build backend


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

@jonahtanjzjonahtanjz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tlylt 👍

Just one suggestion :)

Comment thread.githooks/post-checkout Outdated
@tlylt
tlylt requested a review from ong6August 9, 2022 09:25

@jonahtanjzjonahtanjz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@jonahtanjzjonahtanjz added this to the 4.0.1 milestone Aug 9, 2022
@tlylt
tlylt merged commit 4912410 into MarkBind:masterAug 10, 2022
@ang-zeyu

Copy link
Copy Markdown
Contributor

Not sure if I mentioned this in #1934 as well, but if not then take it as a new point. This tackles only a very small subset of cases if the intention is to streamline the development experience.

Collaborative ones:

  • Fresh repo clone
  • git pull
  • git merge master

Individual ones:

  • git stash pop
  • copy pasting file from some other place on your PC
  • ❔ git is powerful. there are probably a lot more cases where source files can change not related to a checkout

Again, I think we should be providing separate commands that does the build by default to account for all of these. I emphasize separate, so if you don't need to rebuild the typescript files, you don't need to run a watcher -- just build the files once, or not at all, we can let the developer decide based on their individual preferences, workflow (did you do a git stash? merge? ...), PC capabilities (referring the point on tsc watcher being expensive).

While post-checkout is handy, I do not believe we should be relying solely (or at all, for the sake of consistency ("magic" 🎁 that sometimes works but sometimes dosen't)) on it to produce the build artifacts (typescript / anything else) necessary for development. The same goes for cleaning up artifacts in clean.jsvia post-checkout (I only realised this one in retrospect), which does not account for git pull / merge / ....

It also triggers many unnecessary rebuilds / cleans (e.g. checkout to another branch whose HEAD is referencing the same commit).

tldr I think this is ok as an interim solution, I'm still favouring supporting a few separate nuke-everything commands to streamline the development:

// can be less verbose
// the separate commands for the templates can probably be made into cli args as well.
"buildAndDevDocs": "npm run clean && npm run build:backend && node ...index.js s -d -o thedocs",
"buildAndDevDefaultTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o defaulttemplate",
"buildAndDevMinimalTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o minimaltemplate",
// can be less verbose
"watchAndDevDocs": "npm run clean && run the tsc watcher ... && node ...index.js s -d -o thedoc",
"watchAndDevDefaultTemplate": "...",
"watchAndDevMinimalTemplate": "...",
...

@tlylt

Copy link
Copy Markdown
ContributorAuthor

Thanks @ang-zeyu for the detailed comment. Think I should have mentioned more clearly that this is indeed an interim fix in the hope of improving the developer experience.

While not capturing all the cases, especially the unnecessary rebuild when checking out to a new branch whose HEAD is referencing the same commit, it solves most of the issues I faced in the past few weeks of somewhat active development and code review here. For some of the cases listed, they are certainly not in the purview of this PR which makes use of "post-checkout" (and not pull, or merge hooks) that hopefully addresses the case of.......checkout. On the point of "magic", I was thinking that the CLI output of the githook gives quite a bit of hint as to what commands are executed, so it alludes to the underlying working of the script. Of course, this does not replace the details in our DG.

Again, I think we should be providing separate commands that does the build by default to account for all of these. I emphasize separate, so if you don't need to rebuild the typescript files, you don't need to run a watcher -- just build the files once, or not at all, we can let the developer decide based on their individual preferences, workflow (did you do a git stash? merge? ...), PC capabilities (referring the point on tsc watcher being expensive).

tldr I think this is ok as an interim solution, I'm still favouring supporting a few separate nuke-everything commands to streamline the development:

// can be less verbose
// the separate commands for the templates can probably be made into cli args as well.
"buildAndDevDocs": "npm run clean && npm run build:backend && node ...index.js s -d -o thedocs",
"buildAndDevDefaultTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o defaulttemplate",
"buildAndDevMinimalTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o minimaltemplate",
// can be less verbose
"watchAndDevDocs": "npm run clean && run the tsc watcher ... && node ...index.js s -d -o thedoc",
"watchAndDevDefaultTemplate": "...",
"watchAndDevMinimalTemplate": "...",
...

Good points here 👍 would defer to you to put up a tracking issue whenever you are free since I am not entirely sure of any additional details/requirements of the solution you have in mind

@ang-zeyu

Copy link
Copy Markdown
Contributor

🙆‍♂️

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tlylt@ang-zeyu@jonahtanjz
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Update post-checkout hook to auto build backend by tlylt · Pull Request #1994 · MarkBind/markbind · GitHub
Skip to content

Update post-checkout hook to auto build backend - #1994

Merged
tlylt merged 4 commits into
MarkBind:masterfrom
tlylt:update-hooks
Aug 10, 2022
Merged

Update post-checkout hook to auto build backend#1994
tlylt merged 4 commits into
MarkBind:masterfrom
tlylt:update-hooks

Conversation

@tlylt

@tlylttlylt commented Aug 7, 2022

Copy link
Copy Markdown
Contributor

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
Related to discussions in #1934

Anything you'd like to highlight/discuss:
Post checkout hook inputs reference: https://www.atlassian.com/git/tutorials/git-hooks#:~:text=your%20continuous%20integration.-,Post%2DCheckout,-The

Testing instructions:

  • Checkout to this PR branch, the following should be shown
    image
  • Afterwards, can cd docs and markbind serve -d to see that the docs can be served
  • Try modifying the "README.md" file, and discard it via git commit -- README.md, the post checkout hook should not be triggered

Proposed commit message: (wrap lines at 72 characters)
Update post-checkout hook to auto build backend


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

@jonahtanjzjonahtanjz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tlylt 👍

Just one suggestion :)

Comment thread.githooks/post-checkout Outdated
@tlylt
tlylt requested a review from ong6August 9, 2022 09:25

@jonahtanjzjonahtanjz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@jonahtanjzjonahtanjz added this to the 4.0.1 milestone Aug 9, 2022
@tlylt
tlylt merged commit 4912410 into MarkBind:masterAug 10, 2022
@ang-zeyu

Copy link
Copy Markdown
Contributor

Not sure if I mentioned this in #1934 as well, but if not then take it as a new point. This tackles only a very small subset of cases if the intention is to streamline the development experience.

Collaborative ones:

  • Fresh repo clone
  • git pull
  • git merge master

Individual ones:

  • git stash pop
  • copy pasting file from some other place on your PC
  • ❔ git is powerful. there are probably a lot more cases where source files can change not related to a checkout

Again, I think we should be providing separate commands that does the build by default to account for all of these. I emphasize separate, so if you don't need to rebuild the typescript files, you don't need to run a watcher -- just build the files once, or not at all, we can let the developer decide based on their individual preferences, workflow (did you do a git stash? merge? ...), PC capabilities (referring the point on tsc watcher being expensive).

While post-checkout is handy, I do not believe we should be relying solely (or at all, for the sake of consistency ("magic" 🎁 that sometimes works but sometimes dosen't)) on it to produce the build artifacts (typescript / anything else) necessary for development. The same goes for cleaning up artifacts in clean.jsvia post-checkout (I only realised this one in retrospect), which does not account for git pull / merge / ....

It also triggers many unnecessary rebuilds / cleans (e.g. checkout to another branch whose HEAD is referencing the same commit).

tldr I think this is ok as an interim solution, I'm still favouring supporting a few separate nuke-everything commands to streamline the development:

// can be less verbose
// the separate commands for the templates can probably be made into cli args as well.
"buildAndDevDocs": "npm run clean && npm run build:backend && node ...index.js s -d -o thedocs",
"buildAndDevDefaultTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o defaulttemplate",
"buildAndDevMinimalTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o minimaltemplate",
// can be less verbose
"watchAndDevDocs": "npm run clean && run the tsc watcher ... && node ...index.js s -d -o thedoc",
"watchAndDevDefaultTemplate": "...",
"watchAndDevMinimalTemplate": "...",
...

@tlylt

Copy link
Copy Markdown
ContributorAuthor

Thanks @ang-zeyu for the detailed comment. Think I should have mentioned more clearly that this is indeed an interim fix in the hope of improving the developer experience.

While not capturing all the cases, especially the unnecessary rebuild when checking out to a new branch whose HEAD is referencing the same commit, it solves most of the issues I faced in the past few weeks of somewhat active development and code review here. For some of the cases listed, they are certainly not in the purview of this PR which makes use of "post-checkout" (and not pull, or merge hooks) that hopefully addresses the case of.......checkout. On the point of "magic", I was thinking that the CLI output of the githook gives quite a bit of hint as to what commands are executed, so it alludes to the underlying working of the script. Of course, this does not replace the details in our DG.

Again, I think we should be providing separate commands that does the build by default to account for all of these. I emphasize separate, so if you don't need to rebuild the typescript files, you don't need to run a watcher -- just build the files once, or not at all, we can let the developer decide based on their individual preferences, workflow (did you do a git stash? merge? ...), PC capabilities (referring the point on tsc watcher being expensive).

tldr I think this is ok as an interim solution, I'm still favouring supporting a few separate nuke-everything commands to streamline the development:

// can be less verbose
// the separate commands for the templates can probably be made into cli args as well.
"buildAndDevDocs": "npm run clean && npm run build:backend && node ...index.js s -d -o thedocs",
"buildAndDevDefaultTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o defaulttemplate",
"buildAndDevMinimalTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o minimaltemplate",
// can be less verbose
"watchAndDevDocs": "npm run clean && run the tsc watcher ... && node ...index.js s -d -o thedoc",
"watchAndDevDefaultTemplate": "...",
"watchAndDevMinimalTemplate": "...",
...

Good points here 👍 would defer to you to put up a tracking issue whenever you are free since I am not entirely sure of any additional details/requirements of the solution you have in mind

@ang-zeyu

Copy link
Copy Markdown
Contributor

🙆‍♂️

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tlylt@ang-zeyu@jonahtanjz
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Update post-checkout hook to auto build backend by tlylt · Pull Request #1994 · MarkBind/markbind · GitHub
Skip to content

Update post-checkout hook to auto build backend - #1994

Merged
tlylt merged 4 commits into
MarkBind:masterfrom
tlylt:update-hooks
Aug 10, 2022
Merged

Update post-checkout hook to auto build backend#1994
tlylt merged 4 commits into
MarkBind:masterfrom
tlylt:update-hooks

Conversation

@tlylt

@tlylttlylt commented Aug 7, 2022

Copy link
Copy Markdown
Contributor

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
Related to discussions in #1934

Anything you'd like to highlight/discuss:
Post checkout hook inputs reference: https://www.atlassian.com/git/tutorials/git-hooks#:~:text=your%20continuous%20integration.-,Post%2DCheckout,-The

Testing instructions:

  • Checkout to this PR branch, the following should be shown
    image
  • Afterwards, can cd docs and markbind serve -d to see that the docs can be served
  • Try modifying the "README.md" file, and discard it via git commit -- README.md, the post checkout hook should not be triggered

Proposed commit message: (wrap lines at 72 characters)
Update post-checkout hook to auto build backend


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

@jonahtanjzjonahtanjz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tlylt 👍

Just one suggestion :)

Comment thread.githooks/post-checkout Outdated
@tlylt
tlylt requested a review from ong6August 9, 2022 09:25

@jonahtanjzjonahtanjz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@jonahtanjzjonahtanjz added this to the 4.0.1 milestone Aug 9, 2022
@tlylt
tlylt merged commit 4912410 into MarkBind:masterAug 10, 2022
@ang-zeyu

Copy link
Copy Markdown
Contributor

Not sure if I mentioned this in #1934 as well, but if not then take it as a new point. This tackles only a very small subset of cases if the intention is to streamline the development experience.

Collaborative ones:

  • Fresh repo clone
  • git pull
  • git merge master

Individual ones:

  • git stash pop
  • copy pasting file from some other place on your PC
  • ❔ git is powerful. there are probably a lot more cases where source files can change not related to a checkout

Again, I think we should be providing separate commands that does the build by default to account for all of these. I emphasize separate, so if you don't need to rebuild the typescript files, you don't need to run a watcher -- just build the files once, or not at all, we can let the developer decide based on their individual preferences, workflow (did you do a git stash? merge? ...), PC capabilities (referring the point on tsc watcher being expensive).

While post-checkout is handy, I do not believe we should be relying solely (or at all, for the sake of consistency ("magic" 🎁 that sometimes works but sometimes dosen't)) on it to produce the build artifacts (typescript / anything else) necessary for development. The same goes for cleaning up artifacts in clean.jsvia post-checkout (I only realised this one in retrospect), which does not account for git pull / merge / ....

It also triggers many unnecessary rebuilds / cleans (e.g. checkout to another branch whose HEAD is referencing the same commit).

tldr I think this is ok as an interim solution, I'm still favouring supporting a few separate nuke-everything commands to streamline the development:

// can be less verbose
// the separate commands for the templates can probably be made into cli args as well.
"buildAndDevDocs": "npm run clean && npm run build:backend && node ...index.js s -d -o thedocs",
"buildAndDevDefaultTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o defaulttemplate",
"buildAndDevMinimalTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o minimaltemplate",
// can be less verbose
"watchAndDevDocs": "npm run clean && run the tsc watcher ... && node ...index.js s -d -o thedoc",
"watchAndDevDefaultTemplate": "...",
"watchAndDevMinimalTemplate": "...",
...

@tlylt

Copy link
Copy Markdown
ContributorAuthor

Thanks @ang-zeyu for the detailed comment. Think I should have mentioned more clearly that this is indeed an interim fix in the hope of improving the developer experience.

While not capturing all the cases, especially the unnecessary rebuild when checking out to a new branch whose HEAD is referencing the same commit, it solves most of the issues I faced in the past few weeks of somewhat active development and code review here. For some of the cases listed, they are certainly not in the purview of this PR which makes use of "post-checkout" (and not pull, or merge hooks) that hopefully addresses the case of.......checkout. On the point of "magic", I was thinking that the CLI output of the githook gives quite a bit of hint as to what commands are executed, so it alludes to the underlying working of the script. Of course, this does not replace the details in our DG.

Again, I think we should be providing separate commands that does the build by default to account for all of these. I emphasize separate, so if you don't need to rebuild the typescript files, you don't need to run a watcher -- just build the files once, or not at all, we can let the developer decide based on their individual preferences, workflow (did you do a git stash? merge? ...), PC capabilities (referring the point on tsc watcher being expensive).

tldr I think this is ok as an interim solution, I'm still favouring supporting a few separate nuke-everything commands to streamline the development:

// can be less verbose
// the separate commands for the templates can probably be made into cli args as well.
"buildAndDevDocs": "npm run clean && npm run build:backend && node ...index.js s -d -o thedocs",
"buildAndDevDefaultTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o defaulttemplate",
"buildAndDevMinimalTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o minimaltemplate",
// can be less verbose
"watchAndDevDocs": "npm run clean && run the tsc watcher ... && node ...index.js s -d -o thedoc",
"watchAndDevDefaultTemplate": "...",
"watchAndDevMinimalTemplate": "...",
...

Good points here 👍 would defer to you to put up a tracking issue whenever you are free since I am not entirely sure of any additional details/requirements of the solution you have in mind

@ang-zeyu

Copy link
Copy Markdown
Contributor

🙆‍♂️

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tlylt@ang-zeyu@jonahtanjz
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); Update post-checkout hook to auto build backend by tlylt · Pull Request #1994 · MarkBind/markbind · GitHub
Skip to content

Update post-checkout hook to auto build backend - #1994

Merged
tlylt merged 4 commits into
MarkBind:masterfrom
tlylt:update-hooks
Aug 10, 2022
Merged

Update post-checkout hook to auto build backend#1994
tlylt merged 4 commits into
MarkBind:masterfrom
tlylt:update-hooks

Conversation

@tlylt

@tlylttlylt commented Aug 7, 2022

Copy link
Copy Markdown
Contributor

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
Related to discussions in #1934

Anything you'd like to highlight/discuss:
Post checkout hook inputs reference: https://www.atlassian.com/git/tutorials/git-hooks#:~:text=your%20continuous%20integration.-,Post%2DCheckout,-The

Testing instructions:

  • Checkout to this PR branch, the following should be shown
    image
  • Afterwards, can cd docs and markbind serve -d to see that the docs can be served
  • Try modifying the "README.md" file, and discard it via git commit -- README.md, the post checkout hook should not be triggered

Proposed commit message: (wrap lines at 72 characters)
Update post-checkout hook to auto build backend


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

@jonahtanjzjonahtanjz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tlylt 👍

Just one suggestion :)

Comment thread.githooks/post-checkout Outdated
@tlylt
tlylt requested a review from ong6August 9, 2022 09:25

@jonahtanjzjonahtanjz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@jonahtanjzjonahtanjz added this to the 4.0.1 milestone Aug 9, 2022
@tlylt
tlylt merged commit 4912410 into MarkBind:masterAug 10, 2022
@ang-zeyu

Copy link
Copy Markdown
Contributor

Not sure if I mentioned this in #1934 as well, but if not then take it as a new point. This tackles only a very small subset of cases if the intention is to streamline the development experience.

Collaborative ones:

  • Fresh repo clone
  • git pull
  • git merge master

Individual ones:

  • git stash pop
  • copy pasting file from some other place on your PC
  • ❔ git is powerful. there are probably a lot more cases where source files can change not related to a checkout

Again, I think we should be providing separate commands that does the build by default to account for all of these. I emphasize separate, so if you don't need to rebuild the typescript files, you don't need to run a watcher -- just build the files once, or not at all, we can let the developer decide based on their individual preferences, workflow (did you do a git stash? merge? ...), PC capabilities (referring the point on tsc watcher being expensive).

While post-checkout is handy, I do not believe we should be relying solely (or at all, for the sake of consistency ("magic" 🎁 that sometimes works but sometimes dosen't)) on it to produce the build artifacts (typescript / anything else) necessary for development. The same goes for cleaning up artifacts in clean.jsvia post-checkout (I only realised this one in retrospect), which does not account for git pull / merge / ....

It also triggers many unnecessary rebuilds / cleans (e.g. checkout to another branch whose HEAD is referencing the same commit).

tldr I think this is ok as an interim solution, I'm still favouring supporting a few separate nuke-everything commands to streamline the development:

// can be less verbose
// the separate commands for the templates can probably be made into cli args as well.
"buildAndDevDocs": "npm run clean && npm run build:backend && node ...index.js s -d -o thedocs",
"buildAndDevDefaultTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o defaulttemplate",
"buildAndDevMinimalTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o minimaltemplate",
// can be less verbose
"watchAndDevDocs": "npm run clean && run the tsc watcher ... && node ...index.js s -d -o thedoc",
"watchAndDevDefaultTemplate": "...",
"watchAndDevMinimalTemplate": "...",
...

@tlylt

Copy link
Copy Markdown
ContributorAuthor

Thanks @ang-zeyu for the detailed comment. Think I should have mentioned more clearly that this is indeed an interim fix in the hope of improving the developer experience.

While not capturing all the cases, especially the unnecessary rebuild when checking out to a new branch whose HEAD is referencing the same commit, it solves most of the issues I faced in the past few weeks of somewhat active development and code review here. For some of the cases listed, they are certainly not in the purview of this PR which makes use of "post-checkout" (and not pull, or merge hooks) that hopefully addresses the case of.......checkout. On the point of "magic", I was thinking that the CLI output of the githook gives quite a bit of hint as to what commands are executed, so it alludes to the underlying working of the script. Of course, this does not replace the details in our DG.

Again, I think we should be providing separate commands that does the build by default to account for all of these. I emphasize separate, so if you don't need to rebuild the typescript files, you don't need to run a watcher -- just build the files once, or not at all, we can let the developer decide based on their individual preferences, workflow (did you do a git stash? merge? ...), PC capabilities (referring the point on tsc watcher being expensive).

tldr I think this is ok as an interim solution, I'm still favouring supporting a few separate nuke-everything commands to streamline the development:

// can be less verbose
// the separate commands for the templates can probably be made into cli args as well.
"buildAndDevDocs": "npm run clean && npm run build:backend && node ...index.js s -d -o thedocs",
"buildAndDevDefaultTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o defaulttemplate",
"buildAndDevMinimalTemplate": "npm run clean && npm run build:backend && node ...index.js s -d -o minimaltemplate",
// can be less verbose
"watchAndDevDocs": "npm run clean && run the tsc watcher ... && node ...index.js s -d -o thedoc",
"watchAndDevDefaultTemplate": "...",
"watchAndDevMinimalTemplate": "...",
...

Good points here 👍 would defer to you to put up a tracking issue whenever you are free since I am not entirely sure of any additional details/requirements of the solution you have in mind

@ang-zeyu

Copy link
Copy Markdown
Contributor

🙆‍♂️

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tlylt@ang-zeyu@jonahtanjz