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

[relay-compiler] Publish compiler binaries to platform-specific binaries #3759

Closed
wants to merge 1 commit into from

Conversation

alunyov
Copy link
Contributor

@alunyov alunyov commented Jan 17, 2022

Based on this suggestion https://twitter.com/IAmTrySound/status/1479191806420078598 (@TrySound)

I'm using similar to esbuild approach: publish binaries to a standalone packages, and use them as platform-specific optional dependencies in the relay-compiler.

This should reduce the size relay-compiler is using on the node_nodules.

Test Plan:

  • Tested locally on mac with the todo app (manually creating artifacts directory, running gulp release) and referencing local dependencies for relay-compiler in the todo app.

@facebook-github-bot
Copy link
Contributor

@alunyov has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@TrySound
Copy link
Contributor

Awesome, thanks!

@zth
Copy link
Contributor

zth commented Jan 18, 2022

You could also delete the binaries that aren't for the current platform in a post install script. Check this example out, it copies the relevant platform binary in place and removes the rest: https://github.com/zth/rescript-relay/blob/master/packages/rescript-relay/scripts/release-postinstall.js#L103-L133

@kassens
Copy link
Member

kassens commented Jan 18, 2022

Each binary is 10-20mb. Is this really a concern that warrants adding complexity and room for breakage?

@@ -0,0 +1,14 @@
{
"name": "relay-compiler-linux-x64",
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we an autogenerated this as part of build?

@alunyov
Copy link
Contributor Author

alunyov commented Jan 18, 2022

@kassens

Each binary is 10-20mb. Is this really a concern that warrants adding complexity and room for breakage?

My concern is that we potentially will need to add support for more platforms. Look at this list in theesbuild
https://github.com/evanw/esbuild/tree/master/npm

I think it is a reasonable improvement in OSS, the storage may not be not free.

@alunyov
Copy link
Contributor Author

alunyov commented Jan 24, 2022

We've decided not to land this for now. Both solutions: optionalDependencies and postInstall script have issues (see the esbuild discussion: evanw/esbuild#1647).

But I'll keep this open for now, maybe we will need to land this if the size will keep regressing.

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.

6 participants