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

Adopt flat bundles #1590

Open
robrichard opened this Issue Apr 6, 2017 · 13 comments

Comments

Projects
None yet
8 participants
@robrichard
Contributor

robrichard commented Apr 6, 2017

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

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

@robrichard robrichard changed the title from Adopt flat bundles? to Adopt flat bundles Apr 6, 2017

@kassens

This comment has been minimized.

Member

kassens commented Apr 7, 2017

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

This comment has been minimized.

Member

gaearon commented Apr 7, 2017

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 #1321.

@twhid

This comment has been minimized.

Contributor

twhid commented Apr 7, 2017

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

This comment has been minimized.

Member

gaearon commented Apr 7, 2017

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 comment has been minimized.

Contributor

leebyron commented Apr 8, 2017

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

This comment has been minimized.

Contributor

robrichard commented Apr 14, 2017

I opened a PR for this here: #1610

@wincent

This comment has been minimized.

Contributor

wincent commented May 4, 2017

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

This comment has been minimized.

Contributor

yasserkaddour commented May 4, 2017

@wincent unfortunately tree shaking is broken in webpack, see webpack/webpack#2867

@alloy

This comment has been minimized.

Collaborator

alloy commented Jan 29, 2018

Looks like webpack 4 is promising to fix this webpack/webpack#2867 (comment). @robrichard is tree shaking the main motivation for this issue and linked PR?

@gaearon

This comment has been minimized.

Member

gaearon commented Jan 29, 2018

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

This comment has been minimized.

Collaborator

alloy commented Jan 29, 2018

@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

This comment has been minimized.

Contributor

robrichard commented Mar 12, 2018

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 #1321 (comment) 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 97f24f8)
  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

This comment has been minimized.

Collaborator

alloy commented Mar 13, 2018

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment