threepointone/glamor

CommonJS build requires 'process.env' replacement

Closed this issue · 5 comments

Note: this happens with glamor v2 and v3.

I'm bundling an app with Rollup, and using glamor:

import { style, cssFor } from 'glamor';

I get an error at runtime in a browser:

Uncaught ReferenceError: process is not defined
    at sheet.js:60

Since the CommonJS (and ES6) builds don't run through Webpack's ReplacePlugin, process.env still exists.

Not sure the best way to resolve this. Maybe using Babel's transform-inline-variables plugin?

My understanding is that rollup-plugin-replace is meant to handle this.

rollup({
  entry: 'main.js',
  plugins: [
    replace({
      'process.env.NODE_ENV': JSON.stringify( 'production' )
    })
  ]
}).then(...)

If that's not the case, can you get us a concise example project to reproduce with? I'm not intimately familiar with Rollup.

Yeah, I can add rollup-plugin-replace in my build and it'll fix the issue.

But my thought is that the published builds should have already performed this translation, and that the client (me, in this case) shouldn't need to do any further transformations to get it to work.

This is in accordance with the paradigm that users shouldn't have to re-transpile code based on their dependencies' build process.

Thoughts?

So my thought is that the user is using a CommonJS build which works in node without modification. That user is retargetting the build to run in the browser which doesn't support CommonJS, therefore a build step must already be happening to bundle the code. We don't know whether it's being bundled in a development environment or a production environment and use process.env.NODE_ENV to turn development tooling and production optmiziations on/off. IMO filling in the gaps for node globals isn't the same as shipping raw import statements since there is no transpiling necessary. Also, React does this too for it's dev/production builds so it's reasonable to expect that this is being set in a large amount of projects using such module bundling techniques.

Since the point of the paragraph you link to is to "ship code that actually runs in the environments you support" it's worth noting that CommonJS is not browser-supported functionality and works fine in node without adjustments. There is also a UMD build which does not have this requirement (you can drop it into a codepen and it just works, for example) which sounds like what you want since it is pre-built for the browser environment.

I'd be interested to know what you think the solution to this would be but will be closing this issue as we offer multiple build types already.

I think that's fair, particularly if you're viewing the CJS and ES6 builds as targeting Node. It might be worth revisiting as <script type=module> makes its way into browsers but that's a fairly rare case right now.

For now it might be worth noting in the README the transformation required to make the modules browser-ready; I can create a PR if you'd like.

@mattdsteele Happy to accept a PR for this. Perhaps linking to the popular solutions for webpack and rollup (DefinePlugin, rollup-plugin-replace) would be useful too. https://github.com/threepointone/glamor#speedy-mode looks like a good place to expand on the process.env mention.

happy to revisit when <script type=module> hits a critical mass.