WIP: added suggestion for rollup config #9201

travi · 2018-10-18T05:25:26Z

for the gatsby package

this is a suggestion to build a common-js main bundle and an es-module module bundle using rollup. there are a few issues that will need to be worked out, but wanted to get some feedback about whether this is an approach that the team would be interested in first.

for context, this is related to #8821 and a follow-up about jsx being present in the module

for the gatsby package

travi · 2018-10-18T05:34:01Z

packages/gatsby/package.json

@@ -179,6 +182,7 @@
    "build:rawfiles": "copyfiles -u 1 src/internal-plugins/**/raw_* dist",
    "build:cjs": "babel cache-dir --out-dir cache-dir/commonjs --ignore **/__tests__",
    "build:src": "babel src --out-dir dist --source-maps --ignore **/gatsby-cli.js,**/raw_*,**/__tests__",
+    "build:rollup": "rollup -c",


if this is integrated, the build:cjs script above could be removed

travi · 2018-10-18T05:35:42Z

packages/gatsby/rollup.config.js

+    })
+  ],
+  output: [
+    {file: 'lib/index.cjs.js', format: 'cjs', sourcemap: true},


the files produced here are the ones that should be referenced as main and module in the package.json.

the directory and file names can certainly be adjusted to fit your conventions, but i started with these to demonstrate the mechanics to start with.

travi · 2018-10-18T05:38:11Z

.babel-preset.js

@@ -1,7 +1,7 @@
 const r = m => require.resolve(m)

 function preset(context, options = {}) {
-  const { browser = false, debug = false } = options
+  const { browser = false, debug = false, modules = 'commonjs' } = options


the output of the transpilation during the rollup build needs to preserve the es-module syntax so that rollup can properly do the rest of its processing. the default retains the previous behavior, but this allows overriding how the modules are handled.

DSchau

Few things:

This fails (for me?)

[!] Error: 'default' is not exported by cache-dir/public-page-renderer.js
https://rollupjs.org/guide/en#error-name-is-not-exported-by-module-
cache-dir/gatsby-browser-entry.js (10:7)

I think if we were to consider an approach here, I think I'd rather move cache-dir to src/ and then move the resulting dist/cache-dir to the root level, after build step

@pieh is there a reason for shipping raw JSX? I honestly don't know if it's beneficial or just making it harder for consumers of Gatsby in various phases (e.g. unit tests, storybook, etc.)

DSchau · 2018-10-18T17:50:31Z

packages/gatsby/package.json

@@ -138,7 +138,10 @@
    "cross-env": "^5.1.4",
    "lerna": "^2.9.0",
    "nyc": "^7.0.0",
-    "rimraf": "^2.6.1"
+    "rimraf": "^2.6.1",
+    "rollup": "^0.66.6",


DSchau · 2018-10-18T17:57:27Z

packages/gatsby/rollup.config.js

+import autoExternal from 'rollup-plugin-auto-external';
+
+export default {
+  input: 'cache-dir/gatsby-browser-entry.js',


This will fail, unfortunately. We want the files to remain unbundled so that we can conditionally require certain files.

For instance, if we take cache-dir/public-page-renderer we can see the various conditional require statements.

Bundling this for the browser honestly just wouldn't work, I don't think, because it'll trace those dependencies, exclude conditional requires that we'd want in certain phases, and then create a (smaller, but not as functional) bundle file.

fair point. i'm not familiar enough with the approach, so this is some of the detail i was hoping to tease out. there could possibly still be a way to accomplish as a bundle (or multiple), but that could be more of an investment than its worth at this point.

maybe the better approach would be simple transpilation then, similar to what was done for the common js version, but with modules set to false for @babel/env.

travi · 2018-10-18T18:14:52Z

This fails (for me?)

[!] Error: 'default' is not exported by cache-dir/public-page-renderer.js
https://rollupjs.org/guide/en#error-name-is-not-exported-by-module-
cache-dir/gatsby-browser-entry.js (10:7)

yeah, thats one of the issues i mentioned, but could have provided more detail about. that is a solvable issue, but didnt get too invasive with my changes before discussing details a bit more

DSchau · 2018-10-18T18:16:35Z

@travi it's just using a commonjs shim right? In my minimal experience with rollup, I think that's how I fixed it

travi · 2018-10-18T18:54:03Z

it's just using a commonjs shim right?

if i were to fix this, i think i'd actually change it to conditionally export the es-module way. however, to your comment above, that might not be appropriate in this context, since it sounds like that conditional needs to be evaluated at runtime rather than build time.

travi · 2018-10-22T04:31:30Z

i'm happy to update this to just do straight transpilation without bundling, list is done for the common-js version for main.

the update to the preset will still be needed, but we'll need to pass config to the babel cli to use the config with modules set to false. it's not clear to me what the best way to accomplish this would be, though, since the .babelrc is already configured to use the preset with modules set to commonjs.

i'm open to thoughts since my familiarity with the raw babel-cli is rusty after using babel through either rollup or webpack for a while now.

DSchau · 2018-10-22T13:44:50Z

@travi what would you say are using the benefits of rollup here if we may not actually want a bundle? I think using babel and transpiling JSX is a happy medium, most likely. Just want to weigh any possible performance/file-size increase of this approach.

(and we need to get some type of performance measurements in place so we can track this at the CI/PR level; I started on this here -> #9167)

travi · 2018-10-22T14:14:51Z

sorry, i probably wasn't clear enough, but based on what you highlighted about the dynamic imports needing to be possible at runtime, i don't think rollup is a good fit here. i agree that using babel-cli directly to transpile to some common denominator is the best bet for now.

i think using the same preset as is used for the common-js version is a pretty good starting point, but with modules set to false (meaning don't transpile the module syntax). you could potentially decide to flip a few more flags, but its probably not necessary.

the piece that i'm not confident about with my babel-cli rustiness is how to use the same preset with different options for two independent builds from the same directory context. after thinking about it a little more since i posted last night, i did at least remember that BABEL_ENV at least used to be available to enable a level of dynamic config in a .babelrc. if that still exists, we could probably make it work with that approach, but i'm open to alternatives if you know of a better approach.

pieh · 2018-10-22T14:55:35Z

I've tried naive approach of using same babel config we use for browser packages like gatsby-link or gatsby-image before but had to revert that - #6183 . I'm not sure what's good approach here, but if we could only transform jsx without transpiling it to es5 that would work

travi · 2018-11-05T04:24:46Z

so as we mentioned, using rollup to create a full bundle probably isn't the right way forward. instead, i've included a new npm script to transpile, but still leave the es-module syntax intact.

to accomplish this, i reused the existing preset (for now pre-move to the separate package), but with the ability to set modules to false. since i dont think passing config to a preset is possible directly from the cli, i used BABEL_ENV and the env block w/in the .babelrc to use different config for the cjs build and the es build.

i see the preset has been pulled out to a separate package, so if we agree to go this route, i'll create a new PR to enable that preset to take the modules option. would we want the change to be the same as i made here, with commonjs as the default? in addition to that, after that one is merged and published, i'll close out this PR since its gotten pretty stale and open a new version that updates the .babelrc as i have here and add the new script.

i wanted to show a potential (working) approach here since the babel preset was still changeable w/o extra hoops to jump through. i'm happy to discuss any details that might need to change due to conventions i'm unaware of, or any other thoughts before moving further.

DSchau · 2019-01-10T15:51:19Z

@travi sorry - just circling back to this after quite a while.

i see the preset has been pulled out to a separate package, so if we agree to go this route, i'll create a new PR to enable that preset to take the modules option

Let's go this route. I think that's a meaningful change that improves the behavior of that preset (it's now in babel-preset-gatsby-package), because I think that is generally valuable and I know other packages could use that behavior.

In the interim--I think there are a few intractable issues with this PR (specifically that the way gatsby is transpiled now isn't super conducive to bundling with something like Rollup). If there are smaller changes we can provide that fix some of the functionality you're seeing with importing this package in tests/storybook/etc. let's go that route, but for now, I don't think we want to introduce a bundler step.

As such--going to close this out. Please let me know if I can help with anything else, and thanks for the PR!

build(rollup): added suggestion for rollup config

7405585

for the gatsby package

travi requested a review from a team as a code owner October 18, 2018 05:25

travi mentioned this pull request Oct 18, 2018

add: docs for storybook #9120

Merged

travi commented Oct 18, 2018

View reviewed changes

DSchau suggested changes Oct 18, 2018

View reviewed changes

build(es-module): added script to transpile to es-modules

66195e7

DSchau closed this Jan 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: added suggestion for rollup config #9201

WIP: added suggestion for rollup config #9201

travi commented Oct 18, 2018

travi Oct 18, 2018

travi Oct 18, 2018

travi Oct 18, 2018

DSchau left a comment

DSchau Oct 18, 2018

travi Oct 18, 2018

DSchau Oct 18, 2018

travi Oct 18, 2018 •

edited

Loading

travi commented Oct 18, 2018

DSchau commented Oct 18, 2018

travi commented Oct 18, 2018

travi commented Oct 22, 2018

DSchau commented Oct 22, 2018

travi commented Oct 22, 2018

pieh commented Oct 22, 2018

travi commented Nov 5, 2018

DSchau commented Jan 10, 2019

WIP: added suggestion for rollup config #9201

WIP: added suggestion for rollup config #9201

Conversation

travi commented Oct 18, 2018

travi Oct 18, 2018

Choose a reason for hiding this comment

travi Oct 18, 2018

Choose a reason for hiding this comment

travi Oct 18, 2018

Choose a reason for hiding this comment

DSchau left a comment

Choose a reason for hiding this comment

DSchau Oct 18, 2018

Choose a reason for hiding this comment

travi Oct 18, 2018

Choose a reason for hiding this comment

DSchau Oct 18, 2018

Choose a reason for hiding this comment

travi Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

travi commented Oct 18, 2018

DSchau commented Oct 18, 2018

travi commented Oct 18, 2018

travi commented Oct 22, 2018

DSchau commented Oct 22, 2018

travi commented Oct 22, 2018

pieh commented Oct 22, 2018

travi commented Nov 5, 2018

DSchau commented Jan 10, 2019

travi Oct 18, 2018 •

edited

Loading