Skip to content
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

Revert typescript migration for core/src/Site/index #2331

Merged
merged 2 commits into from
Jul 17, 2023

Conversation

tlylt
Copy link
Contributor

@tlylt tlylt commented Jul 16, 2023

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:
Fix discovered issue of markbind deploy failure after v5 release.

After migrating to v5, markbind deploy will result in the following error:

$ markbind deploy


| / | __ _ _ __ | | __ | __ ) () _ __ __| |
| |/| | / | | '__| | |/ / | _ \ | | | '_ \ / _ |
| | | | | (
| | | | | < | |
) | | | | | | | | (| |
|
| || _,| || ||_\ |/ || || || _,|

v5.0.0
info: Website generation started at 4:20:07 pm
info: Building assets...
info: Assets built
info: Generating pages...
[======] 6 / 6 pages built
info: Pages built
info: Site data built
info: Website generation complete! Total build time: 1.165s
info: Build success!
error: uncaughtException: Cloning into '....\markbind\node_modules\gh-pages.cache\github.com!tlylt!mb-test.git'...
fatal: '' is not a valid remote name
date=Sun Jul 16 2023 16:20:08 GMT+0800 (Singapore Standard Time), pid=23264, uid=null, gid=null, cwd=C:\Users\User\Documents\GitHub\mb-test\5, execPath=C:\Program Files\nodejs\node.exe, version=v16.19.1, argv=[C:\Program Files\nodejs\node.exe, C:\Program Files\nodejs\node_modules\markbind-cli\index.js, deploy], rss=102879232, heapTotal=74731520, heapUsed=47658840, external=1187571, arrayBuffers=77791, loadavg=[0, 0, 0], uptime=14381
ProcessError: Cloning into '....\markbind\node_modules\gh-pages.cache\github.com!tlylt!mb-test.git'...
fatal: '' is not a valid remote name

at ChildProcess.<anonymous> (C:\Users\User\Documents\GitHub\markbind\node_modules\gh-pages\lib\git.js:42:16)
at ChildProcess.emit (node:events:513:28)
at maybeClose (node:internal/child_process:1100:16)
at Process.ChildProcess._handle.onexit (node:internal/child_process:304:5)

I could not figure out the root cause for this at the moment, but I suspect it's due to the following packages: gh-pages or simple-git.
After revisiting different commits in the git history, I found out that markbind deploy still works in this commit: c4bda22

The PR that broke the deploy functionality seems to be #2270 Migrate Site/index.js to TypeScript. Out of all the changes within the PR, the update of index.js to index.ts seems to be causing the problem. Therefore, I am reverting the change by:

  • reverting index.ts back to index.js
  • fix eslint and gitignore changes
  • fix import export within index.js and other files that imports Site from index.js

To revert index.js, I directly copied from the version before the migrating in #2270, and only adjusted a few lines of code within to accommodate later changes that impacted this file. They are:

They are found from inspecting the history of changes for index.ts file, at here

Anything you'd like to highlight/discuss:
We should definitely look into the original typescript migration and figure out the exact lines that cause this issue.

Side note, I noticed we updated @types/gh-pages to v3 in the original index.ts migration, however we are still using gh-pages@v2. Was wondering if that is the root cause but nope, updating that doesn't help.

Testing instructions:
To see the problem:

  • checkout master (or v5.0.0 tag if this PR has been merged)
  • set up a test repo that has github page publishing configured
  • run markbind deploy

To see the fix:

  • checkout this PR
  • set up a test repo or use one that you already have (rmb to add in the repo detail in site.json, e.g.
  "deploy": {
    "message": "Dev Site Update.",
    "repo": "https://github.com/tlylt/mb-test.git",
    "branch": "gh-pages"
  }
  • run markbind deploy

The two sites at https://markbind.org/ and https://markbind.org/devdocs/devGuide/devGuide.html are deployed using this PR branch, can also verify the output by looking around in there.

Proposed commit message: (wrap lines at 72 characters)
Revert typescript migration for core/src/Site/index

The migration of Site/index.ts to Site/index.js is causing markbind
deploy command to fail with an unexpected error.

Let's first fully revert the migration for index.ts and further investigate
to find ways to migrate this file again.

The index.ts contain way too much content to be able to dissect immediately.
We should provide this work around first as markbind deploy is quite
essential for users that publish their website using this command.


Checklist: ☑️

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

@tlylt tlylt modified the milestone: v5.0.1 Jul 16, 2023
@tlylt tlylt marked this pull request as ready for review July 16, 2023 11:53
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned in the PR description, this file is essentially the original index.js, the reverse operation of 63ed852#diff-1579d51321badfb0388ed10d49bfa7ee76f2983bcbc4efdfb7e4733a8871700e (besides some necessary changes)

@tlylt tlylt requested a review from a team July 16, 2023 11:56
@tlylt tlylt changed the title revert typescript migration for core/src/Site/index Revert typescript migration for core/src/Site/index Jul 16, 2023
Copy link
Contributor

@ong6 ong6 left a comment

Choose a reason for hiding this comment

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

Lgtm

@tlylt tlylt added this to the v5.0.1 milestone Jul 17, 2023
@tlylt tlylt merged commit 835798d into MarkBind:master Jul 17, 2023
6 checks passed
@tlylt tlylt deleted the fix-deploy-error branch July 17, 2023 12:59
@tlylt tlylt restored the fix-deploy-error branch July 17, 2023 12:59
@WillCWX WillCWX mentioned this pull request Jul 24, 2023
11 tasks
Sign up for free to 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.

None yet

2 participants