markbind deploy error after migrating Site/index.js to TypeScript

[tlylt]

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: Migrate Site/index.js to TypeScript #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

[WillCWX]

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.

[tlylt]

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

[WillCWX]

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'

[tlylt]

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?

[WillCWX]

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

[tlylt]

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'?

[WillCWX]

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;
 }