markbind deploy error after migrating Site/index.js to TypeScript
Closed this issue · 7 comments
Please confirm that you have searched existing issues in the repo
Yes, I have searched the existing issues
Any related issues?
No response
Tell us about your environment
Windows 10
MarkBind version
Master branch
Describe the bug and the steps to reproduce it
Issue discovered after V5.0.0 is released. See detailed info and what has been done about it in this follow-up PR: #2331
TLDR:
- users cannot call markbind deploy while other functionalities seems alright
- by reverting the Site/index.ts back to Site/index.js (plus required changes), the issue seems fixed
- we still want to get to the root of things, namely:
- review the original migration PR: #2270
- find out which part causes the markbind deploy issue
- re-migrate index.js back to index.ts, ensuring all changes have been applied properly
- possibly add a test case to ensure markbind deploy is properly tested
Expected behavior
No response
Anything else?
No response
const options: DeployOptions = {
branch: this.siteConfig.deploy.branch || defaultDeployConfig.branch,
message: this.siteConfig.deploy.message || defaultDeployConfig.message,
repo: this.siteConfig.deploy.repo || defaultDeployConfig.repo,
remote: '',
};from here
It seems that the issue is due to the remote being '' rather than undefined, resulting in the error of fatal: '' is not a valid remote name. We already have a default for remote in the defaultDeployConfig which is 'origin' so we can just use that. The error seems to be a typo / careless mistake.
Proposed fix:
- remote: '',
+ remote: defaultDeployConfig.remote,if we want users to be able to define their remote, then some extra lines are needed in SiteConfig.ts
Deployed website as proof of fix:
https://willcwx.github.io/ghpages-test
I can't find any other causes as I'm not getting anymore errors after this fix.
Side note:
- I'm unsure if a test case is possible for this. Given that the issue here is that no remote named
''exists. - Perhaps if we allow users to define their remote, we can have a negative test case for an invalid remote.
- We should also probably handle this exception in a nicer way.
Thanks @WillCWX, it does look like the culprit here.
I was suspecting that line but did not pursue it further as I was thinking the remote attribute might be passed in from somewhere else, but it seems like it is not being exposed to the public at all. Also because I was wondering why this existing test case did not fail:
Re your points:
if we want users to be able to define their remote, then some extra lines are needed in SiteConfig.ts
We don't need it now if not affecting anything and not being requested by any user
I'm unsure if a test case is possible for this. Given that the issue here is that no remote named '' exists.
It's ok if we can't add another test but we should investigate why the existing test case is passing (false positive?)
Proposed fix:
Would you like to go ahead and raise a PR? 👀, take note of typescript migration steps in https://markbind-master.netlify.app/devguide/development/migratingtotypescript
Would you like to go ahead and raise a PR? 👀, take note of typescript migration steps in https://markbind-master.netlify.app/devguide/development/migratingtotypescript
Can I confirm that I will be making brand new migration commits instead of reverting the revert?
I was suspecting that line but did not pursue it further as I was thinking the remote attribute might be passed in from somewhere else, but it seems like it is not being exposed to the public at all. Also because I was wondering why this existing test case did not fail:
TLDR:
The test passes because the mocked ghpages.publish() is unable to throw any errors as it did when we used deploy, code here. Furthermore, options.remote is modified after publish() and causes it to pass the test. code here
The code is broken up a lot so its difficult to follow but here is what it looks like together:
// begin
deploy(ciTokenVar: string | boolean) {
const defaultDeployConfig: DeployOptions = {
branch: 'gh-pages',
message: 'Site Update.',
repo: '',
remote: 'origin', // default is correct
};
process.env.NODE_DEBUG = 'gh-pages';
return this.generateDepUrl(ciTokenVar, defaultDeployConfig);
}
// next
async generateDepUrl(ciTokenVar: boolean | string, defaultDeployConfig: DeployOptions) {
const publish = Bluebird.promisify(ghpages.publish);
await this.readSiteConfig();
const depOptions = await this.getDepOptions(ciTokenVar, defaultDeployConfig, publish); // causes error
return Site.getDepUrl(depOptions, defaultDeployConfig); // causes options.remote to be 'origin'
}
// function that causes wrong options.remote
async getDepOptions(ciTokenVar: boolean | string, defaultDeployConfig: DeployOptions,
publish: (basePath: string, options: DeployOptions) => Bluebird<unknown>) {
const options: DeployOptions = {
branch: this.siteConfig.deploy.branch || defaultDeployConfig.branch,
message: this.siteConfig.deploy.message || defaultDeployConfig.message,
repo: this.siteConfig.deploy.repo || defaultDeployConfig.repo,
remote: '', // error here
};
// ......... code that doesn't edit options.remote
// sets ghpages.options
await publish(basePath, options); // should throw an error but can't since it is mocked
return options;
}
// then this is ran and fixes options.remote
static getDepUrl(options: DeployOptions, defaultDeployConfig: DeployOptions) {
const git = simpleGit({ baseDir: process.cwd() });
// fixes options.remote to be correct
options.remote = defaultDeployConfig.remote;
return Site.getDeploymentUrl(git, options);
}
// as options is passed by reference throughout
ghpages.options.remote == 'origin'Can I confirm that I will be making brand new migration commits instead of reverting the revert?
Yes, basically re-migrate this file, taking reference from the original migration + retaining the few lines of changes that I updated in my revert (which were due to other PRs),
The test passes because the mocked ghpages.publish() is unable to throw any errors as it did when we used deploy, code here. Furthermore, options.remote is modified after publish() and causes it to pass the test. code here
So is it fixable?
Yes, basically re-migrate this file, taking reference from the original migration + retaining the few lines of changes that I updated in my revert (which were due to other PRs),
Ok I'll work on it
So is it fixable?
Yes just do this at here:
- options.remote = defaultDeployConfig.remote;I'll do the fix in the same PR
So is it fixable?
Yes just do this at here:
- options.remote = defaultDeployConfig.remote;
Just wondering... if this is the case, why isn't options.remote updated correctly to 'origin', since even as we incorrectly assigned it to '', it is still being overridden here back to 'origin'?
Just wondering... if this is the case, why isn't options.remote updated correctly to 'origin', since even as we incorrectly assigned it to '', it is still being overridden here back to 'origin'?
The error occurs at this function at publish which is before the overriding line that updates options.remote correctly to origin
ghpages.publish actually commits and pushes the branch to the remote (github). Since we used await, the function will run to completion first before options.remote is correctly updated to origin and hence will cause the error that the remote is not valid.
async getDepOptions(ciTokenVar: boolean | string, defaultDeployConfig: DeployOptions, publish: (basePath: string, options: DeployOptions) => Bluebird<unknown>) { const options: DeployOptions = { branch: this.siteConfig.deploy.branch || defaultDeployConfig.branch, message: this.siteConfig.deploy.message || defaultDeployConfig.message, repo: this.siteConfig.deploy.repo || defaultDeployConfig.repo, remote: '', // error here }; // This throws an error! as options.remote is not valid! await publish(basePath, options); return options; }