Adopt flat bundles

[robrichard]

Are there plans to use rollup and generate flat bundles for Relay (similar to what react did in https://github.com/facebook/react/pull/9327)? It would be nice to get smaller bundles and it looks like this would also address the performance regression in server side rendering (https://github.com/facebook/relay/issues/1321).

Is this something that could be accepted as a pull request without access to internal FB infrastructure?

[kassens]

We are a few days away from Making Travis Green Again and since the bundling/building is not used internally, this could be a great external contribution. I'll give @leebyron a chance to chime in who know the node landscape a bit better.

[gaearon]

FWIW we still require() fbjs modules in the React flat bundle. Is there any downside to this? I’m not sure I understand the discussion in https://github.com/facebook/relay/issues/1321.

[twhid]

Re: #1321

Having the requires inline, particularly if you're using require hooks (eg babel/register), slows down a node server considerably. We saw a ~75% decrease in server rendering time when we switched to a custom relay build that hoists the require calls to the top of the scope.

[gaearon]

Oh I see. We do hoist them in our React flat bundles. They look like:

// all flattened modules below are using those
var invariant = require('fbjs/lib/invariant');
var warning = require('fbjs/lib/warning');

/* code */

[leebyron]

This would be great! Right now we're using webpack to create these, but I agree that using Rollup would be better. I think this is a great task for someone looking to contribute to work on

[robrichard]

I opened a PR for this here: https://github.com/facebook/relay/pull/1610

[wincent]

Right now we're using webpack to create these, but I agree that using Rollup would be better.

Haven't used them myself yet, but webpack (as of v2) also has some support for tree-shaking features pioneered in Rollup.

[yasserkaddour]

@wincent unfortunately tree shaking is broken in webpack, see https://github.com/webpack/webpack/issues/2867

[alloy]

Looks like webpack 4 is promising to fix this https://github.com/webpack/webpack/issues/2867#issuecomment-353714614. @robrichard is tree shaking the main motivation for this issue and linked PR?

[gaearon]

I don't think so. This is beneficial for many reasons. In our case the main benefits were caching process.env.NODE_ENV access in SSR, being able to inline more helpers using GCC simple mode, protecting internals from being imported. I wrote a post about this:

https://reactjs.org/blog/2017/12/15/improving-the-repository-infrastructure.html#compiling-flat-bundles

[alloy]

@gaearon That’s great info, thanks 👍

@kassens Then I guess the question remains, is it possible to adopt ES6 modules in Relay from FB’s perspective?

[robrichard]

I'd like to start working on this again. I'm going to abandon the current (new very outdated) PR and start fresh. I'm very interested in getting this working as it would solve the server side rendering performance degradation described in https://github.com/facebook/relay/issues/1321#issuecomment-361308163 without degrading client side performance.

I think the most incremental approach would be separate PRs that do something like this:

1. Ensure all modules and types are imported using relative paths (complete what was started in https://github.com/facebook/relay/commit/97f24f8ebe80ebfd849db8e88f8449bfc9f9f5eb)
2. Publish the files in the same directory structure that they are in /packages (instead of all files in /lib and remove the build transformation steps that rewrite the imports.
3. Update all requires to ESM imports/exports.
4. Update the build process to produce flat bundles using rollup.

@jstejada @alloy do you think this makes sense? Is it realistic for these changes to be merged? Would any of this interfere with how FB consumes this package?

[alloy]

I obviously can’t speak for FB, but the order of things makes sense to me 👍

Changing to ESM would also eliminate the one runtime addition that my language plugin PR adds.

Might be a good idea to check if the jest-haste package is able to output all the paths of resolved haste modules?

[kassens]

@robrichard the first 2 steps sound right. In fact, I think I already completed number 1.

2 makes sense to me and would allow us to clean that ugly transform rewriting requires. Would be an annoying, but not terrible breaking change for folks directly requireing the deeper modules, but seems fine.

3 we cannot do yet as we have an environment that only consumes requires and we use the Relay master verbatim there. What we could do is adopt Rollup first (there's some plugin to consume requires that I think React is using too?). After we have that, I can see if we can consume the bundled version internally (I hope it reduces overhead of the many modules!). If it should work internally and we just consume the flat bundle, we can do the last step and convert to import.

Hope that makes sense!

[jstejada]

@ruslanvs please stop adding these comments

[liweinan0423]

s

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.