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

Add rollup-plugin-replace to replace process.env in React builds #3

Closed
wants to merge 1 commit into from

Conversation

nguyensomniac
Copy link

Love using Figplug but always wished the React scaffold would work :( This fixes the error seen in #2, where the imported React library errors out because process is undefined.

I tested this end-to-end with the default React scaffold, and things are working again in Figma!

Copy link
Owner

@rsms rsms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
I’ll test this locally tomorrow to be sure, but it looks good to me.
I’d also first like to find a repro for the issue and update test.sh so that it fails, and then apply this patch to see the tests pass.

@@ -768,6 +769,9 @@ export class Product {
let incfg = {
input: this.entry,
plugins: [
replacePlugin({
'process.env.NODE_ENV': c.debug ? JSON.stringify('development') : JSON.stringify('production')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder if the calls to JSON.stringify can be replaced with just constants, e.g. '”development"'

Copy link
Owner

@rsms rsms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. It seems the culprit is that the latest react library tries to unconditionally access process.env.NODE_ENV which is of course not set in a web browser.

The nicest fix here is to conditionally declare process at the end of lib/figplug.js

Reasons for doing that instead of using :

  1. Always try to avoid NPM modules when possible.
  2. rollup-plugin-replace seems to replace strings, which is always a tricky thing to do in generated code. Figplug applies some constant optimizations, so code like "process.env" + "NODE_ENV" becomes "process.env.NODE_ENV” which would be replaced by "production" which turns out as invalid JavaScript (i.e. ""production"”)

Something like this, at the end of lib/figplug.js, should do it:

// Finally, set process to make things like process.env.NODE_ENV not explode.
if (typeof process == "undefined") {
  const process = { env: { NODE_ENV: DEBUG ? "production" : "development" } }
} else if (!process.env) {
  process.env = { NODE_ENV: DEBUG ? "production" : "development" }
}

Let me know if you’d like me to make this small change, or if you’d like to update your PR to this :–)

@rsms
Copy link
Owner

rsms commented Oct 23, 2019

I just tested this theory (it works) but it’s not ideal.
The constant evaluator can’t guarantee that an object property doesn’t change at runtime, so it is unable to eliminate code like this:

if (process.env.NODE_ENV !== "production") {
// lots of helper code
}

Which brings us back to your idea with just replacing strings in the code.
I wonder if we can somehow replace this at the AST level instead, so that legitimate text containing the string isn’t replaced...

@rsms rsms closed this in daec9d3 Oct 23, 2019
@rsms
Copy link
Owner

rsms commented Oct 23, 2019

Okay, I’ve got a good fix for this. Sorry that this PR didn’t land! I really appreciate the time you took to help out :-)

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

Successfully merging this pull request may close these issues.

2 participants