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

Create flat bundles using rollup #1610

Closed
wants to merge 1 commit into from

Conversation

robrichard
Copy link
Contributor

Updated the gulpfile to use Rollup to generate commonjs and umd bundles as discussed here: #1590. I followed the pattern used by the master branch of React for requiring the dev or prod enabled versions.

I'm being very explicit with regard to what modules are external vs included in the bundles. Let me know there is a different way you would like to handle it.

Also what is the best way I can test that these bundles are built correctly and working as expected?

@robrichard
Copy link
Contributor Author

Hey @leebyron @kassens - are you still considering using Rollup to build the new relay packages as discussed in #1590? If so it would be good to get it in before the final 1.0.0 release, otherwise it will be a breaking change. It would removes access to internal modules, e.g. require('react-relay/lib/toGraphQL')

@kassens
Copy link
Member

kassens commented Apr 19, 2017

Interesting, hadn't thought of that. This would probably be a good thing though as we should expose all necessary things publicly.

@alloy
Copy link
Contributor

alloy commented Jan 28, 2018

@kassens @jstejada Reading the history of this PR, it does seem like there was interest on FB’s side to merge such an external contribution. Does that still apply?

@robrichard
Copy link
Contributor Author

I did try to rebase that PR a few months ago and ran into some issues with the dynamic requires of the relay dev tools.

Also it would be much more beneficial if the relay source was updated to use ES modules as was done in the React source code. By using rollup-plugin-commonjs here, each module will still be wrapped in a closure.

@alloy
Copy link
Contributor

alloy commented Jan 28, 2018

@robrichard Is that something that can be done iteratively?

@robrichard
Copy link
Contributor Author

@alloy hard for me to say as I don't know how it would mesh with FB's custom module system.

@alloy
Copy link
Contributor

alloy commented Jan 28, 2018

I meant more, can this PR continue as-is and discuss ES6 modules as a next step or is blocked by that by now?

Also, out of curiosity, does React not use the Haste module system?

@robrichard
Copy link
Contributor Author

Yes this change would not be blocked by that.

React was converted to ES modules in these two PRs:
facebook/react#11303
facebook/react#11389

@robrichard
Copy link
Contributor Author

Going to close this as there are a few issues with this approach:

  1. The rollup config is complicated by the way files are imported without relative file paths.
  2. The source files use CommonJS, meaning that rollup still requires wrapping them in a closure function, meaning the final bundle is not truly flat.

@robrichard robrichard closed this Mar 12, 2018
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.

4 participants