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

Add CodeSandbox CI Config #17175

Merged
merged 7 commits into from
Oct 31, 2019
Merged

Add CodeSandbox CI Config #17175

merged 7 commits into from
Oct 31, 2019

Conversation

CompuIves
Copy link
Contributor

@CompuIves CompuIves commented Oct 24, 2019

This will enable CodeSandbox CI on the React repo. To have a better idea of what CodeSandbox CI does you can read more about it here: https://u2edh.csb.app/. I recommend merging this PR first before installing the GitHub App, to ensure that all future PRs will work from the get-go.

The gist of it is:

The goal of this app is to make it easier for you to test your library without publishing to npm yet. Since CodeSandbox is already used for bug reports we wanted to make it possible to also test the PR version of your library with the existing bug reports.

In an ideal flow, it would work like this:

  1. Someone opens an issue for a bug with a sandbox reproducible
  2. Someone opens a fix PR mentioning the issue
  3. We build the library from the PR, fork the sandbox repro and install the new library in that sandbox
    This way you will only have to open the generated sandbox to confirm that the fix works. No need to clone, install or test locally.

@sizebot
Copy link

sizebot commented Oct 24, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS against a4e12fd

@@ -0,0 +1,9 @@
{
"packages": ["packages/react", "packages/react-dom"],
"buildCommand": "build react/index,react-dom/index",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"buildCommand": "build react/index,react-dom/index",
"buildCommand": "build --type=UMD,NODE react/index,react-dom/index",

The other bundles won't be published anyway due to the publishDirectory.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't need UMD, though

"buildCommand": "build react/index,react-dom/index",
"publishDirectory": {
"react": "build/node_modules/react",
"react-dom": "build/node_modules/react-dom"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's do Scheduler, too.

@acdlite
Copy link
Collaborator

acdlite commented Oct 28, 2019

We already build all the packages and store them as CircleCI artifacts. Is there some way we could leverage those instead of building twice? Then we don't have to worry about them getting out of sync. These are also the same artifacts we publish to npm.

Perhaps by waiting for CircleCI to finish first, and then downloading the tarball from there?

"publishDirectory": {
"react": "build/node_modules/react",
"react-dom": "build/node_modules/react-dom",
"scheduler": "build/node_modules/scheduler"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this will cause the same issue as mui/material-ui#17976 (comment):
react-dom declares a dependency on scheduler with the newly created version but codesandbox thinks this version is on the npm registry.

I guess this applies regardless of whether scheduler is built in the codesandbox CI or not.

@acdlite
Copy link
Collaborator

acdlite commented Oct 29, 2019

react-dom declares a dependency on scheduler with the newly created version but codesandbox thinks this version is on the npm registry.

The way we handle this with our prerelease builds is we have a post-build step to rewrite the package jsons to update the version numbers to point to each other:

// This method is used by both local Node release scripts and Circle CI bash scripts.
// It updates version numbers in package JSONs (both the version field and dependencies),
// As well as the embedded renderer version in "packages/shared/ReactVersion".
// Canaries version numbers use the format of 0.0.0-<sha> to be easily recognized (e.g. 0.0.0-57239eac8).
// A separate "React version" is used for the embedded renderer version to support DevTools,
// since it needs to distinguish between different version ranges of React.
// It is based on the version of React in the local package.json (e.g. 16.6.1-canary-57239eac8).
// Both numbers will be replaced if the canary is promoted to a stable release.
const updateVersionsForCanary = async (cwd, reactVersion, version) => {

So then the result looks like:

"name": "react-dom",
"version": "0.0.0-f6b8d31a7",
"dependencies": {
  "loose-envify": "^1.1.0",
  "object-assign": "^4.1.1",
  "prop-types": "^15.6.2",
  "scheduler": "0.0.0-f6b8d31a7"
},
"peerDependencies": {
  "react": "0.0.0-f6b8d31a7"
}

We can do the same in the build script for CodeSandbox CI. (Also this is one of the reasons I wanted to pull from the CircleCI artifacts, so subtle stuff like this stays in sync.)

Seems like it could be a cool feature for CodeSandbox CI to support, too :)

@eps1lon
Copy link
Collaborator

eps1lon commented Oct 29, 2019

Yeah I tried this too but the issue is that codesandbox looks for this version on the npm registry. Unless you publish the PR build to npm codesandbox won't find it. Unless they changed it in the last days.

The key difference is that react-dom is declared in the package.json of the codesandbox as a url not a version string.

@acdlite
Copy link
Collaborator

acdlite commented Oct 29, 2019

Ah yeah that's exactly the same I issue I ran into when I tried to do a version of this myself. I was using tarball URLs and the top level dependencies loaded fine, but the transitive dependencies didn't because CodeSandbox tried to pull them from npm.

This is a blocker for us being able to use it, unfortunately

@CompuIves
Copy link
Contributor Author

It should resolve the package with the same version number (regardless of the source, npm or csb), since we use the node file resolver for finding packages. If that's not happening we encountered a bug, I'll take a look at that today!

I really like the idea of version rewriting, I'll take a look at that today as well!

@CompuIves
Copy link
Contributor Author

CompuIves commented Oct 30, 2019

I added version rewriting to the CI service, it should go live in a couple hours. This should make it easier for packages that require eachother to resolve eachother.

I also found why we resolve other dependency versions for transitive dependencies, it's interesting, because yarn has the same behaviour as CodeSandbox, but npm resolves them from the tar files. I got it to work locally and in CodeSandbox by using a resolution field, but I like the rewriting of the versions more as it's more transparent (and it feels less fragile).

@CompuIves
Copy link
Contributor Author

You can see the current version of the PR in action in CodeSandbox CI here:

@acdlite
Copy link
Collaborator

acdlite commented Oct 30, 2019

Thanks so much for this PR, @CompuIves! And for CodeSandbox! Excited to merge this tomorrow

@CompuIves
Copy link
Contributor Author

Everything should be working as expected now! I'm super excited for this 😄 😄

@acdlite
Copy link
Collaborator

acdlite commented Oct 31, 2019

Ok let's go! Thank you again!

@acdlite acdlite merged commit a1ff9fd into facebook:master Oct 31, 2019
@acdlite
Copy link
Collaborator

acdlite commented Oct 31, 2019

I'll need to ask the OSS team at Facebook to approve the GitHub app. But I don't expect that will be an issue :)

trueadm pushed a commit to trueadm/react that referenced this pull request Nov 4, 2019
* Add CodeSandbox CI Config

* Add default sandbox to build

* Make build more efficient and add scheduler

* Force build

* Add scheduler image

* Add scheduler/tracing to the build

* Force another build
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants