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

fix: Allow Docusaurus and IDE co-existence #916

Merged
merged 2 commits into from Mar 21, 2022
Merged

fix: Allow Docusaurus and IDE co-existence #916

merged 2 commits into from Mar 21, 2022

Conversation

cmumatt
Copy link
Contributor

@cmumatt cmumatt commented Mar 18, 2022

Description

This PR allows Docusaurus and IDE co-existence at CloudFlare by injecting the IDE build output into the Docusaurus build, which eliminates the need for two deployments and two URLs.

Checklist

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new ESLint warnings
  • I have reviewed any generated changes to the diagrams/ folder

Open questions

Questions that require more discussion or to be addressed in future development:

  • I temporarily commented out the live demo since it is just showing a circle; I figured that's still a work in progress.

@cmumatt cmumatt self-assigned this Mar 18, 2022
@cmumatt cmumatt changed the title Allow Docusaurus and IDE co-existence fix: Allow Docusaurus and IDE co-existence Mar 18, 2022
@codecov
Copy link

codecov bot commented Mar 18, 2022

Codecov Report

Merging #916 (2f6ee06) into main (36c0867) will not change coverage.
The diff coverage is n/a.

❗ Current head 2f6ee06 differs from pull request most recent head 3777fad. Consider uploading reports for the commit 3777fad to get more accurate results

@@           Coverage Diff           @@
##             main     #916   +/-   ##
=======================================
  Coverage   68.17%   68.17%           
=======================================
  Files          62       62           
  Lines        8117     8117           
  Branches     1768     1768           
=======================================
  Hits         5534     5534           
  Misses       2576     2576           
  Partials        7        7           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36c0867...3777fad. Read the comment docs.

@cloudflare-pages
Copy link

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 3777fad
Status: ✅  Deploy successful!
Preview URL: https://10528a16.penrose-panes.pages.dev

View logs

@cmumatt cmumatt requested review from samestep and wodeni March 19, 2022 18:31
@cmumatt cmumatt marked this pull request as ready for review March 20, 2022 16:43
Copy link
Member

@wodeni wodeni left a comment

Choose a reason for hiding this comment

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

Thanks for doing this one! Still a little unclear to me how the deployment process look like. Are we using Cloudflare now? Looking at it now, it's still deploying panels. In my head I was thinking we were going to:

  • Add a top-level script in package.json: yarn build:sites builds everything, right now just panels and docs-site
  • This script should build @penrose/docs-site and @penrose/panels, and then collect build artifacts into a singular folder for deployment.

Lmk if I was just thinking about the wrong thing, and how deployment is done at the moment.

@@ -6,7 +6,7 @@
"docusaurus": "docusaurus",
"start": "NODE_OPTIONS='--max-old-space-size=8192' docusaurus start",
"watch": "NODE_OPTIONS='--max-old-space-size=8192' docusaurus start",
"build": "NODE_OPTIONS='--max-old-space-size=8192' docusaurus build",
"build": "rm -f ./static/try/*;cp ../panels/public/* ./static/try/;NODE_OPTIONS='--max-old-space-size=8192' docusaurus build",
Copy link
Member

Choose a reason for hiding this comment

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

Would be better to have a separate script for this. You could use a pre-hook (prebuild) for this purpose: https://docs.npmjs.com/cli/v8/using-npm/scripts#pre--post-scripts

Copy link
Member

Choose a reason for hiding this comment

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

Also, rimraf is supposedly faster and platform independent for rm -rf

Copy link
Contributor Author

@cmumatt cmumatt Mar 21, 2022

Choose a reason for hiding this comment

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

Awesome! Thanks for reviewing and for that suggestion; I also like the idea of a pre-hook better and created issue #917 to implement that. At the same time, I will check if there is a similar analog for cp if we aren't assuming a *ix-like script environment.

Cloudflare is temporarily still deploying penrose/main to the old penrose-panels project -- but all traffic to that project is redirected to the penrose-site project (penrose.cs.cmu.edu), which currently hosts this PR's build output. So, CD is currently broken on purpose. Once this PR is merged, I plan to re-point penrose/main's CD to penrose-site, which involves re-creating both projects due to repo associations being 1:1 and immutable at CF, as you pointed out previously, which is kind of a bummer -- but our current reality.

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