V2: Inline process.browser for better code elimination

[garthenweb]

↪️ Pull Request

Migrates #2583 to v2. This feature is heavily used in some projects and can save a lot of dead code to be eliminated.

I made two changes compared to v1:

• The process.browser = true assignment that might be somewhere in the code is not replaced anymore but always set to true. Removing this line could lead to breaking the code in rare situations, but not changing it too true would be inconsequent as well. Let me know if you have any thoughts about that.
• The replacement should also be done for Node and Electron. I think there is no need to keep any reference as those should always be static. process.browser is different from process.env which can change after build time on the server. Tests for Node and Electron as skipped for now, as all those tests were skipped so far. I expect this to be implemented later for v2.

💻 Examples

See #2583.

🚨 Test instructions

Place process.browser somewhere in your code and see it is replaced with true when build for the target browser.

[mischnic]

The process.browser = true assignment

Why would you set that? If a bundler doesn't support it and/or without a bundler?

[garthenweb]

The process.browser = true assignment

Why would you set that? If a bundler doesn't support it and/or without a bundler?

When not in a browser, I would not expect this line to be in the code at all, as it is only inserted when polyfilling node process: https://github.com/defunctzombie/node-process/blob/master/browser.js#L156
By default in other environments, this should be undefined. But you could just add it by hand. In case we want to transform all usages, this change would be the most consistent.

I can bearly find use cases for this, so it is really hard to decide on that. Maybe this could harm in a test environment where you want to mock browser behavior?
Maybe it is better to not touch it at all in none browser environments, what do you think?

[mischnic]

Not sure if we should replace it on node as well. It isn't a "standardized" property on process and purely added by bundlers / the polyfill you linked.

But you could just add it by hand.

Is that common (where the value differs from node-process/browser.js)? I hope not.

process.browser should be true for context: electron-renderer and false for context: electron-main. Here the definitions for env.isNode() and env.isBrowser()

parcel/packages/core/core/src/public/Environment.js

Lines 15 to 24 in 7cf6a3b

	const BROWSER_ENVS = new Set([
	'browser',
	'web-worker',
	'service-worker',
	'electron-renderer'
	]);
	const ELECTRON_ENVS = new Set(['electron-main', 'electron-renderer']);
	const NODE_ENVS = new Set(['node', ...ELECTRON_ENVS]);
	const WORKER_ENVS = new Set(['web-worker', 'service-worker']);
	const ISOLATED_ENVS = WORKER_ENVS;

[garthenweb]

Okay, I agree with not touching it on NodeJS and updated the PR.
Is there any way I can activate the tests for NodeJS? At the moment asset.env.isNode() always returns false. I tried to configure the environment but without success.

I guess electron is handled with asset.env.isNode() as well? I am a bit unsure now how to configure the tests so that they make sense and pass 😕 Would be happy for an advice!

[mischnic]

I am a bit unsure now how to configure the tests

Either adding a integration-tests/test/integration/process/package.json with:

{
	"main": "dist/node.js",
	"browser": "dist/browser.js",
	"electronMain": "dist/electron.js",
	"targets": {
		"electronMain": {
			"context": "electron-main"
		},
		"electronRenderer": {
			"context": "electron-renderer"
		}
	}
}

or passing this as the second argument to bundle (though not 100% sure if that works):
{ targets: { main: { context: "node"}} (or default instead of main)

[garthenweb]

Alright, tests are in place now. Unfortunately, electron-main and electron-renderer are no supported context targets, it throws here:

parcel/packages/core/test-utils/src/utils.js

Line 186 in 7cf6a3b

throw new Error('Unknown target ' + target);

Is this expected or a bug? Based on the types there is a mismatch as far as I can see:

parcel/packages/core/types/index.js

Line 76 in 7cf6a3b

export type EnvironmentContext =

[mischnic]

Yeah, that was never updated from Parcel 1. It should be:

    case 'browser': 
		...
    case 'node':
	case 'electron-main':
		....
    case 'electron-renderer':
		...
    default:
      throw new Error('Unknown target ' + target);

Could you please update that as well?

[garthenweb]

Okay, I hope that everything works now.
I fixed the test utils as advised and also reactivated the environment tests for electron. I assume the same behavior we see for the process.browser applies to process.env, let me know if I am wrong with this.

Thank you for your support on this :)

[mischnic]

same behavior we see for the process.browser applies to process.env

For electron-renderer, process.env should not be replaced though.

[garthenweb]

same behavior we see for the process.browser applies to process.env

For electron-renderer, process.env should not be replaced though.

@mischnic Alright, I pushed my next try. I hope everything is in place now!

[DeMoorJasper]

@garthenweb could you rebase this PR on V2?

[garthenweb]

Sure, will have a look on the weekend!

[garthenweb]

@DeMoorJasper The branch is rebased and up to date now. Tests are passing. I squashed the commits as this was the easiest way to rebase.

Let me know if something is missing!

[DeMoorJasper]

Looks good to me, thanks for rebasing this so quickly :)