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

import from process left behind in browser-targeted output #7904

Closed
mindplay-dk opened this issue Apr 5, 2022 · 4 comments · Fixed by #7954
Closed

import from process left behind in browser-targeted output #7904

mindplay-dk opened this issue Apr 5, 2022 · 4 comments · Fixed by #7954

Comments

@mindplay-dk
Copy link

🐛 bug report

I'm trying to build a "universal" ES6 library - meaning, it should be able to load in modern Node, modern browsers, and probably Deno too.

The addition of browserslist appears to resolve all process.env references statically - however, one reference looks like it was left behind, and the resulting module leads with:

import {platform as $gh3jN$platform, env as $gh3jN$env} from "process";

Shouldn't there be an error or warning for this, when the bundler knows it's building for a browser context, and the resulting build still has a Node specific import left in it?

It appears the lingering process import comes from the ansi-colors, which contains this:

  if ('FORCE_COLOR' in process.env) {
    colors.enabled = process.env.FORCE_COLOR !== '0';
  }

Perhaps the in operator is not supported by static analysis?

Or maybe the static analysis is getting confused by the presence of two nested tests?

🎛 Configuration (.babelrc, package.json, cli command)

My attempted package.json (and the project) is located here.

🤔 Expected Behavior

Probably at least a warning or error, for either one or both of these:

  1. Unable to remove all process references from a browser target.
  2. Unable to handle a process.env reference via static analysis for any reason.

Ideally, static analysis should probably be enhanced to support this though? It looks like both rollup and webpack are capable of handling this case, so it would probably be best to have parity with that, as it's rather influential on the ecosystem already.

😯 Current Behavior

(see above)

💁 Possible Solution Workaround

I could submit a PR to ansi-colors and try to make it more compatible with Parcel's static analysis. (assuming it's possible to somehow rewrite that process.env check in a way that works the same?)

🔦 Context

(see project referenced above.)

💻 Code Sample

(see references above.)

🌍 Your Environment

Software Version(s)
Parcel parcel@2.4.1, @parcel/packager-ts@2.4.1, @parcel/transformer-typescript-types@2.4.1
Node v16.14.2
npm/Yarn n/a
Operating System Ubuntu 16.04.7 LTS, (Release: 16.04, Codename: xenial)
@mischnic
Copy link
Member

mischnic commented Apr 5, 2022

if ('FORCE_COLOR' in process.env)

We could definitely support that: https://github.com/parcel-bundler/parcel/blob/v2/packages/transformers/js/core/src/env_replacer.rs

@mindplay-dk
Copy link
Author

Unfortunately, I don't know Rust...

mindplay-dk added a commit to mindplay-dk/inertion that referenced this issue Apr 6, 2022
mindplay-dk added a commit to mindplay-dk/ansi-colors that referenced this issue Apr 11, 2022
Parcel is currently [unable to perform static analysis](parcel-bundler/parcel#7904) on the `in` operator applied to `process.env`, for the purposes of bundling the module. (Parcel is a front-end to SWC, so this most likely affects SWC as well.)

Would you mind making this very minor change?

It should work exactly the same - `enabled` is `true` by default, so `FORCE_COLOR` only has an effect when set to exactly `0`.

It's a bit simpler and avoids mutations, so maybe a bit easier to read anyhow. 🙂
@mindplay-dk
Copy link
Author

I've tested the 2.5.0 release, and it still leaves behind the platform import.

import{platform as e}from"process";

I've pushed it to a branch here.

Apparently it's due to these references in ansi-colors here:

const isWindows = process.platform === 'win32';
const isLinux = process.platform === 'linux';

Is it supposed to handle platform as well as env?

(Should I file a separate issue for this?)

@mischnic
Copy link
Member

Is it supposed to handle platform as well as env?

Currently, it doesn't (so this is more of a feature request than a bug report).

(Should I file a separate issue for this?)

Yes

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

Successfully merging a pull request may close this issue.

2 participants