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

DevTools build script enhancements #17653

Merged
merged 5 commits into from
Dec 18, 2019

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Dec 18, 2019

Resolves #17629

  • Adds a helper script to download the latest experimental build from CI.
  • Updates DevTools build instructions to reference this new script as well as the dependency on the experimental release channel.

New command:

./scripts/release/download-experimental-build.js

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 18, 2019

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 430db52:

Sandbox Source
distracted-grothendieck-p086f Configuration

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

Nice. Can we also update react-devtools README to point to these other READMEs for development? I keep forgetting which package to look at, so I check react-devtools first.

@bvaughn
Copy link
Contributor Author

bvaughn commented Dec 18, 2019

Sure

Copy link
Contributor

@threepointone threepointone left a comment

Choose a reason for hiding this comment

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

🚢

```sh
yarn build -- react,react-dom,react-is,scheduler --type=NODE
RELEASE_CHANNEL=experimental \
Copy link
Contributor

Choose a reason for hiding this comment

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

I keep making versions of this line for myself too, seems like a useful enough command to have its own alias in scripts? (Maybe not in this PR, and I’m happy to do so myself)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I'll add a new alias to package for DevTools. Not sure that's exactly what you're suggesting, but it may be worth doing anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Lemme think about it some more? Just seems so common, but also tribal/hearsay knowledge.

Copy link
Contributor Author

@bvaughn bvaughn Dec 18, 2019

Choose a reason for hiding this comment

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

Okay. For now though I added a yarn build-for-devtools alias because it's silly that everyone should have to remember the exact set of dependencies.


Once the above packages have been built, you can build the extension by running:
#### Download from CI
To use the latest build from CI, run the following command from the root of the repository:
Copy link
Contributor

Choose a reason for hiding this comment

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

It’s not clear that this is an alternative to the previous step.

Copy link
Contributor Author

@bvaughn bvaughn Dec 18, 2019

Choose a reason for hiding this comment

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

Why not? They're subheaders under the same header which says,

You'll need to either build or download those packages first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could make the header size smaller maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a problem with just having one of them? Like, downloading from CI could be the suggested option, and you have a postscript that mentions that’s you can also build from source if needed.

I’m thinking from perspective of someone following instructions to build the extension, who doesn’t understand what these deps are, or what context they provide. If I had to quickly make a build and followed these instructions, I might have done both steps. Ofc this is just a nit, so feel free to ignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It's not sufficient.

We probably want to build from source, so we can test both DevTools and React changes.

External contributors (or Mozilla reviewers) probably want to download a CI build because it's faster and their purpose is not to test React.

@sizebot
Copy link

sizebot commented Dec 18, 2019

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 430db52

@sizebot
Copy link

sizebot commented Dec 18, 2019

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 430db52

@bvaughn bvaughn merged commit 95056b6 into facebook:master Dec 18, 2019
@bvaughn bvaughn deleted the devtools-build-script-enhancements branch December 18, 2019 22:34
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.

Update DevTools build process to use artifacts from CI
5 participants