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

Possible regression with require('stream') when bundling for browser in 0.11.16 #1220

Closed
geoffharcourt opened this issue Apr 28, 2021 · 5 comments

Comments

@geoffharcourt
Copy link

geoffharcourt commented Apr 28, 2021

Hi, first off, this is an amazing project and the speed at which it's moving is really amazing. We are about to start building our app in production with esbuild and it solves a bunch of issues we've had with resource constraints, etc.

Here's our issue, which isn't hugely urgent, but looks like a regression.

Our app (React, browser-based) builds successfully in 0.11.15, but fails in 0.11.16:

 > node_modules/react-dom/cjs/react-dom-server.node.production.min.js:10:74: error: Could not resolve "stream" (use "platform: 'node'" when building for node)
    10 │ 'use strict';var k=require("object-assign"),m=require("react"),aa=require("stream");function r(a){for(var b="https://reactjs.org/docs/error-decoder.html?invariant="+a,c=1;c<arguments.length;c++)b+="&args[]="+encodeURIComponent(arguments[c]);return"Minified React...
       ╵                                                                           ~~~~~~~~

 > node_modules/react-dom/cjs/react-dom-server.node.development.js:21:21: error: Could not resolve "stream" (use "platform: 'node'" when building for node)
    21 │ var stream = require('stream');
       ╵                      ~~~~~~~~

Failed! Error: Build failed with 2 errors:
node_modules/react-dom/cjs/react-dom-server.node.development.js:21:21: error: Could not resolve "stream" (use "platform: 'node'" when building for node)
node_modules/react-dom/cjs/react-dom-server.node.production.min.js:10:74: error: Could not resolve "stream" (use "platform: 'node'" when building for node)

We tried adding stream-browserify and mapping stream to it in package.json without any change. Here's some other stuff from our config that might be relevant:

  bundle: true,
  define: {
    global: 'globalThis',
    // Shim for react-table-fixed-columns
    __webpack_require__: 'webpackShim',
  },
  entryNames: '[dir]/[name]-[hash]',
  entryPoints: files.reduce(
    (accumulator, filename) => {
      accumulator.push(`app/javascript/packs/${filename}`);

      return accumulator;
    },
    [],
  ),
  inject: [
    processShim,
    'app/javascript/lib/esbuild/webpackShim.ts',
  ],
  loader: {
    '.eot': 'file',
    '.jpg': 'file',
    '.png': 'file',
    '.svg': 'file',
    '.ttf': 'file',
    '.vtt': 'file',
    '.woff': 'file',
    '.woff2': 'file',
  },
  metafile: true,
  minify: false,
  outdir: 'public/builds',
  platform: 'browser',
  publicPath: '/builds',
  sourcemap: tre,
  sourcesContent: true,
  target: 'es2015',
@evanw
Copy link
Owner

evanw commented Apr 30, 2021

Option 1: avoid the problem

It looks like the react-dom package is already browser-friendly. Specifically it has server.js, server.node.js, and server.browser.js with server.js mapped to server.browser.js via package.json:

  "browser": {
    "./server.js": "./server.browser.js"
  },

You are bundling react-dom/cjs/react-dom-server.node.development.js and react-dom/cjs/react-dom-server.node.production.min.js so I'm guessing you're explicitly importing server.node.js since server.node.js looks like this:

if (process.env.NODE_ENV === 'production') {
  module.exports = require('./cjs/react-dom-server.node.production.min.js');
} else {
  module.exports = require('./cjs/react-dom-server.node.development.js');
}

If your app uses React and is browser-based, you could try importing react-dom/server or react-dom/server.browser instead of importing react-dom/server.node. That should avoid this problem entirely since the server.browser.js package doesn't import stream at all.

Option 2: reproduce the problem

If you would still like to fix this then I'm going to need a way to reproduce this myself so that I can add it to my growing test suite of browser field test cases. The browser field is pretty janky because there's no specification anywhere for the algorithm it uses. It's just a convention that all bundlers implement differently. So I've been just trying to have esbuild match what other bundlers do for all test cases that I've come across so far. Having another test case for this would be valuable.

I tried constructing a test case for this that looks like this:

$ echo 'require("react-dom/server.node")' > entry.js
$ echo '{"browser":{"stream":"stream-browserify"}}' > package.json
$ npm i react react-dom stream-browserify [email protected]

However, esbuild version 0.11.15 isn't able to build this, so I was unable to reproduce your issue:

$ node_modules/.bin/esbuild --bundle entry.js
 > node_modules/react-dom/cjs/react-dom-server.node.development.js:18:21: error: Could not resolve "stream" (use "--platform=node" when building for node)
    18 │ var stream = require('stream');
       ╵                      ~~~~~~~~

@geoffharcourt
Copy link
Author

Thanks for your response @evanw! I will see if I can reproduce this in a smaller case. We're only doing import React from 'react' throughout our application, but it's possible one of our dependencies is pulling in server.node.js somewhere.

@geoffharcourt
Copy link
Author

geoffharcourt commented Apr 30, 2021

OK, we found it. We do this in a component:

import { renderToString } from 'react-dom/server';

but for some reason we're pulling in the node version despite the browser mapping for the package. We have bundle: true and platform: 'browser'. Still working on a test case.

We removed the instances in our application, this looks like it's coming from dependencies. We have several dependencies that do something like this:

import { renderToStaticMarkup } from 'react-dom/server'

@evanw
Copy link
Owner

evanw commented Apr 30, 2021

Another idea is to potentially try looking at the output of logLevel: 'verbose' to see where and why server.node.js was chosen.

@geoffharcourt
Copy link
Author

geoffharcourt commented Apr 30, 2021

We figured it out. We have a package for SSR that imports the node version of react-dom/server, react-on-rails. It's curious that it didn't cause failures prior to 0.11.16, but this helps resolve why this was happening. Thanks for your advice and your time!

We finished migrating our application to use esbuild in production this afternoon and we're extremely excited about it. Thanks also for your time building this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants