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

feat(v2): official CodeSandbox support #3717

Merged
merged 14 commits into from
Nov 16, 2020
Merged

feat(v2): official CodeSandbox support #3717

merged 14 commits into from
Nov 16, 2020

Conversation

sammychinedu2ky
Copy link
Contributor

@sammychinedu2ky sammychinedu2ky commented Nov 10, 2020

Motivation

I saw an issue on adding Codesandbox config for Docusaurus which would enable the creation of Codesandbox templates for Docusaurus.
So I decided to work on that.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

I added a dev script to the package.json file and a sandbox.config.json file for each template under the examples folder
I made use of the classic template to create a docusaurus template on codesandbox. You can check it out using this link https://codesandbox.io/s/docusaurusclassics-xekwd
I generated the examples folder using this script
mkdir -p examples && cd examples && rm -rf * && npx @docusaurus/init@next init classic classic && npx @docusaurus/init@next init facebook facebook && npx @docusaurus/init@next init bootstrap bootstrap
Then added the dev script and sandbox.config.json file manually

Related PRs

#3702

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 10, 2020
@netlify
Copy link

netlify bot commented Nov 10, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 3868e8f

https://deploy-preview-3717--docusaurus-2.netlify.app

@slorber slorber added the pr: new feature This PR adds a new API or behavior. label Nov 12, 2020
@slorber slorber changed the title feat:added support for codesandbox feat(v2): added support for codesandbox Nov 12, 2020
@slorber slorber linked an issue Nov 12, 2020 that may be closed by this pull request
@slorber
Copy link
Collaborator

slorber commented Nov 12, 2020

Your codesandbox seems to work, but for merging this I'll need:

  • to have a script in monorepo root package.json that "regenerates" the examples
  • not using npx @docusaurus/init@next init because this will fetch the init cli script from npm. We'd rather ensure that we use the locally build init script to be sure to run the latest one. If I'm on a branch doing local modifications to a template, I should be able to run the regenerate command and see my template modifications appearing immediately in the examples, so that my PR include the updated examples immediately.
  • CI tests must pass. If ESlint complains because of the missing FB header, add the failing paths to .eslintignore

Please explain with all the details how did you create that codesandbox template. I must be able to test your whole workflow, considering I know nothing about codesandbox.
I tried to import your fork's URL on Codesandbox, but it didn't work and I have no idea what to do about it.
Also, it's unclear to me how Codesandbox understand which theme it should run.

Please document the whole workflow I need to follow to validate that your work is fine and ready to be merged. Don't assume I know Codesandbox at all, because you probably know more than me

@sammychinedu2ky
Copy link
Contributor Author

sammychinedu2ky commented Nov 12, 2020

Ok cool would work on a script that generates it without npm.
It won't work from my forks URL because I made use of the myforksurl/examples/classic as the entry point on code sandbox

@sammychinedu2ky
Copy link
Contributor Author

Would detail everything once I make a script

@slorber
Copy link
Collaborator

slorber commented Nov 12, 2020

Thanks, It looks like indeed importing an url like https://github.com/sammychinedu2ky/docusaurus/tree/master/examples/facebook seems to work better :)

@sammychinedu2ky
Copy link
Contributor Author

Yes that's is because the entry point has less than 500 modules
But my question is should I create a file specifically for the script .. or should I use the packages docusaurus command to do it locally?

@slorber
Copy link
Collaborator

slorber commented Nov 12, 2020

Not sure it's the best way, but it seems the following work: node ./packages/docusaurus-init/bin/index.js init examples/classic classic

You could add scripts in root package.json that just do that + cleaning folder + copy the required json file

@sammychinedu2ky
Copy link
Contributor Author

sammychinedu2ky commented Nov 12, 2020 via email

@sammychinedu2ky
Copy link
Contributor Author

I've added a script for that in the package.json file (examples:generate)

"last 1 safari version"
]
}
"name": "examples-classic",
Copy link
Collaborator

Choose a reason for hiding this comment

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

why use 1 space indentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that would change it to 2

"hardReloadOnChange": true,
"view": "browser",
"template": "node"
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you generate it in the script instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yh sure let me do that

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Thanks, that looks great but can't run yarn examples:generate twice without error (need to cleanup)

@sammychinedu2ky
Copy link
Contributor Author

Thanks, that looks great but can't run yarn examples:generate twice without error (need to cleanup)

OK let me work on that

@sammychinedu2ky
Copy link
Contributor Author

I've worked on the fix. Everything is working now.

Copy link
Collaborator

@slorber slorber 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 your work, going to merge it.

I did some changes on your code because:

  • you reverted some important package.json changes
  • your cleanup of example folder did not work reliably
  • some ESLint rules didn't pass
  • all files should have the FB copyright header
  • the node init script should rather forward its logs to parent process, so that users knows something is happening

Otherwise it works fine.

I'd recommend checking your Github workflow, because it's really bad if you end up reverting some changes others have done in your PR, like it happened here

package.json Outdated
@@ -40,7 +41,7 @@
"test": "jest",
"test:build:v2": "./admin/scripts/test-release.sh",
"watch": "yarn lerna run --parallel --no-private watch",
"clear": "yarn workspace docusaurus-2-website clear && yarn lerna exec --ignore docusaurus yarn rimraf lib",
"clear": "yarn workspace docusaurus-2-website clear && yarn lerna exec 'yarn rimraf lib' --ignore docusaurus",
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should not be in your PR

package.json Outdated
"@babel/core": "^7.9.0",
"@babel/plugin-proposal-nullish-coalescing-operator": "^7.8.3",
"@babel/plugin-proposal-optional-chaining": "^7.9.0",
"@babel/preset-typescript": "^7.9.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should not be in your PR

@slorber slorber changed the title feat(v2): added support for codesandbox feat(v2): official CodeSandbox support Nov 16, 2020
@slorber slorber merged commit bb33f54 into facebook:master Nov 16, 2020
@sammychinedu2ky
Copy link
Contributor Author

thanks a lot, Sir, I'm grateful for the corrections but I tried pulling from the master branch before creating a new branch locally to work on. I think I made a mistake in that.
But is there any other thing that made the pull request revert changes others have done?

@sammychinedu2ky
Copy link
Contributor Author

Thanks for merging this pull request too 😁

@slorber
Copy link
Collaborator

slorber commented Nov 16, 2020

NP

I don't know exactly what happened 😅 just know that some diff was bad and had to manually revert it.

Can you try to document this new Codesandbox support? In Readme and docs etc?

(just wait until I merge #3760)

@sammychinedu2ky
Copy link
Contributor Author

NP

I don't know exactly what happened just know that some diff was bad and had to manually revert it.

Can you try to document this new Codesandbox support? In Readme and docs etc?

(just wait until I merge #3760)

Ok sure would do that. Thanks again 😁

@lex111 lex111 added this to the v2.0.0-alpha.67 milestone Nov 17, 2020
@bravo-kernel
Copy link
Contributor

Awesome addition 👏

slorber added a commit that referenced this pull request Jul 22, 2021
* feat:added support for codesandbox

* added sandbox config file

* feat:added script to generate template for codesandbox

* added examples:generate script to package.json file

* added failing path to eslintignore

* added script to eslintignore

* added cleaning feature to script

* deleted sandbox script in root

* changed comment in codesandboxscript file

* update codesandboxscript.js + revert bad package.json changes

* eslint should check codesandboxscript

Co-authored-by: slorber <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v2] Proposal: Official CodeSandbox Template
5 participants