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

Fix dev packager global reference #9814

Merged
merged 1 commit into from
Jun 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 81 additions & 2 deletions packages/core/integration-tests/test/globals.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import assert from 'assert';
import path from 'path';
import {bundle, run} from '@parcel/test-utils';

describe('global alias', function () {
import {bundle, fsFixture, overlayFS, run} from '@parcel/test-utils';
import sinon from 'sinon';

describe('globals', function () {
it('should support global alias syntax', async function () {
let b = await bundle(
path.join(__dirname, '/integration/global-alias/index.js'),
Expand All @@ -19,4 +21,81 @@ describe('global alias', function () {
'ok',
);
});

describe('supports global in imported modules', () => {
const dir = path.join(__dirname, 'global-var');

beforeEach(async () => {
await overlayFS.mkdirp(dir);
await fsFixture(overlayFS, dir)`
index.js:
import { main } from './main';

onGlobal(main());

main.js:
export function main() {
let _global = typeof global !== 'undefined' ? global : 'missing global';

return _global;
}

yarn.lock: {}
`;
});

afterEach(async () => {
await overlayFS.rimraf(dir);
});

it('when scope hoisting is disabled', async function () {
let bundleGraph = await bundle(path.join(dir, 'index.js'), {
defaultTargetOptions: {
context: 'browser',
shouldScopeHoist: false,
},
inputFS: overlayFS,
});

let bundles = await Promise.all(
bundleGraph
.getBundles()
.map(b => overlayFS.readFile(b.filePath, 'utf8')),
);

let onGlobal = sinon.spy();
await run(bundleGraph, {globalThis: 'global', onGlobal});

assert(bundles.some(b => b.includes('var global = arguments[3]')));
assert(
bundles.every(b => !b.includes('var $parcel$global = globalThis')),
);
assert.equal(onGlobal.callCount, 1);
assert.deepEqual(onGlobal.firstCall.args, ['global']);
});

it('when scope hoisting is enabled', async function () {
let bundleGraph = await bundle(path.join(dir, 'index.js'), {
defaultTargetOptions: {
context: 'browser',
shouldScopeHoist: true,
},
inputFS: overlayFS,
});

let bundles = await Promise.all(
bundleGraph
.getBundles()
.map(b => overlayFS.readFile(b.filePath, 'utf8')),
);

let onGlobal = sinon.spy();
await run(bundleGraph, {globalThis: 'global', onGlobal});

assert(bundles.some(b => b.includes('var $parcel$global = globalThis')));
assert(bundles.every(b => !b.includes('var global = arguments[3]')));
assert.equal(onGlobal.callCount, 1);
assert.deepEqual(onGlobal.firstCall.args, ['global']);
});
});
});
8 changes: 4 additions & 4 deletions packages/core/integration-tests/test/sourcemaps.js
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,7 @@ describe('sourcemaps', function () {
source: inputs[0],
generated: raw,
str: 'const local',
generatedStr: 'let o',
generatedStr: 'let r',
sourcePath: 'index.js',
});

Expand All @@ -457,7 +457,7 @@ describe('sourcemaps', function () {
source: inputs[0],
generated: raw,
str: 'local.a',
generatedStr: 'o.a',
generatedStr: 'r.a',
sourcePath: 'index.js',
});

Expand All @@ -466,7 +466,7 @@ describe('sourcemaps', function () {
source: inputs[1],
generated: raw,
str: 'exports.a',
generatedStr: 't.a',
generatedStr: 'o.a',
sourcePath: 'local.js',
});

Expand All @@ -475,7 +475,7 @@ describe('sourcemaps', function () {
source: inputs[2],
generated: raw,
str: 'exports.count = function(a, b) {',
generatedStr: 't.count=function(e,n){',
generatedStr: 'o.count=function(e,n){',
sourcePath: 'utils/util.js',
});
});
Expand Down
2 changes: 1 addition & 1 deletion packages/packagers/js/src/DevPackager.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ export class DevPackager {
let output = code || '';
wrapped +=
JSON.stringify(this.bundleGraph.getAssetPublicId(asset)) +
':[function(require,module,exports) {\n' +
':[function(require,module,exports,__globalThis) {\n' +
Copy link
Member

Choose a reason for hiding this comment

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

Where will that __globalThis be referenced?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is referenced here

I should have added it to the description, my bad :)

Copy link
Member

Choose a reason for hiding this comment

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

that's referenced via arguments[3] though. Wouldn't this mean that if someone declared let __globalThis in their code they'd get an error stating that it is already defined? Doesn't look like this variable is used directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's correct. I opted for __globalThis since __global is more likely to be defined. I'd prefer to use globalThis directly instead of specifying an argument since it's well supported, but it would be inconsistent with the parcel runtime.

output +
'\n},';
wrapped += JSON.stringify(deps);
Expand Down
2 changes: 1 addition & 1 deletion packages/packagers/js/src/dev-prelude.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@
localRequire,
module,
module.exports,
this
globalObject
);
}

Expand Down
Loading