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

Correctly set worker's output format if not support by environment #6534

Merged
merged 5 commits into from
Jul 2, 2021
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 10 additions & 2 deletions packages/core/core/src/requests/TargetRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,9 @@ export class TargetResolver {
context: descriptor.context,
isLibrary: descriptor.isLibrary,
includeNodeModules: descriptor.includeNodeModules,
outputFormat: descriptor.outputFormat,
outputFormat:
descriptor.outputFormat ??
this.options.defaultTargetOptions.outputFormat,
shouldOptimize:
this.options.defaultTargetOptions.shouldOptimize &&
descriptor.optimize !== false,
Expand Down Expand Up @@ -291,6 +293,7 @@ export class TargetResolver {
context: 'browser',
engines: {},
shouldOptimize: this.options.defaultTargetOptions.shouldOptimize,
outputFormat: this.options.defaultTargetOptions.outputFormat,
shouldScopeHoist: this.options.defaultTargetOptions
.shouldScopeHoist,
sourceMap: this.options.defaultTargetOptions.sourceMaps
Expand Down Expand Up @@ -590,6 +593,7 @@ export class TargetResolver {

let outputFormat =
descriptor.outputFormat ??
this.options.defaultTargetOptions.outputFormat ??
inferredOutputFormat ??
(targetName === 'module' ? 'esmodule' : 'commonjs');
let isModule = outputFormat === 'esmodule';
Expand Down Expand Up @@ -753,7 +757,10 @@ export class TargetResolver {
context: descriptor.context,
includeNodeModules: descriptor.includeNodeModules,
outputFormat:
descriptor.outputFormat ?? inferredOutputFormat ?? undefined,
descriptor.outputFormat ??
this.options.defaultTargetOptions.outputFormat ??
inferredOutputFormat ??
undefined,
isLibrary: descriptor.isLibrary,
shouldOptimize:
this.options.defaultTargetOptions.shouldOptimize &&
Expand All @@ -779,6 +786,7 @@ export class TargetResolver {
env: createEnvironment({
engines: pkgEngines,
context,
outputFormat: this.options.defaultTargetOptions.outputFormat,
shouldOptimize: this.options.defaultTargetOptions.shouldOptimize,
shouldScopeHoist: this.options.defaultTargetOptions.shouldScopeHoist,
sourceMap: this.options.defaultTargetOptions.sourceMaps
Expand Down
1 change: 1 addition & 0 deletions packages/core/core/src/resolveOptions.js
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ export default async function resolveOptions(
publicUrl,
distDir,
engines: initialOptions?.defaultTargetOptions?.engines,
outputFormat: initialOptions?.defaultTargetOptions?.outputFormat,
},
};
}
1 change: 1 addition & 0 deletions packages/core/core/src/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ export type ParcelOptions = {|
+publicUrl: string,
+distDir?: FilePath,
+engines?: Engines,
+outputFormat?: OutputFormat,
|},
|};

Expand Down

Large diffs are not rendered by default.

85 changes: 80 additions & 5 deletions packages/core/integration-tests/test/javascript.js
Original file line number Diff line number Diff line change
Expand Up @@ -851,15 +851,17 @@ describe('javascript', function() {
assert(/new SharedWorker(.*?, {[\n\s]+type: 'module'[\n\s]+})/.test(main));
});

for (let scopeHoist of [true, false]) {
for (let shouldScopeHoist of [true, false]) {
it(`should compile workers to non modules if ${
scopeHoist ? 'browsers do not support it' : 'scopeHoist = false'
shouldScopeHoist
? 'browsers do not support it'
: 'shouldScopeHoist = false'
}`, async function() {
let b = await bundle(
path.join(__dirname, '/integration/workers-module/index.js'),
{
defaultTargetOptions: {
shouldScopeHoist: true,
shouldScopeHoist,
engines: {
browsers: '>= 0.25%',
},
Expand All @@ -869,14 +871,22 @@ describe('javascript', function() {

assertBundles(b, [
{
assets: ['dedicated-worker.js', 'index.js'],
assets: [
'dedicated-worker.js',
!shouldScopeHoist && 'esmodule-helpers.js',
'index.js',
].filter(Boolean),
},
{
name: 'index.js',
assets: ['index.js', 'bundle-url.js', 'get-worker-url.js'],
},
{
assets: ['shared-worker.js', 'index.js'],
assets: [
!shouldScopeHoist && 'esmodule-helpers.js',
'shared-worker.js',
'index.js',
].filter(Boolean),
},
]);

Expand Down Expand Up @@ -905,6 +915,71 @@ describe('javascript', function() {
});
}

for (let supported of [false, true]) {
it(`should compile workers to ${
supported ? '' : 'non '
}modules when browsers do ${
supported ? '' : 'not '
}support it with esmodule parent script`, async function() {
let b = await bundle(
path.join(__dirname, '/integration/workers-module/index.js'),
{
mode: 'production',
defaultTargetOptions: {
engines: {browsers: supported ? 'Chrome 80' : 'Chrome 75'},
outputFormat: 'esmodule',
shouldScopeHoist: true,
shouldOptimize: false,
},
},
);

assertBundles(b, [
{
type: 'js',
assets: ['dedicated-worker.js'],
},
{
name: 'index.js',
assets: ['index.js', 'bundle-url.js', 'get-worker-url.js'],
},
{
type: 'js',
assets: ['shared-worker.js'],
},
{
type: 'js',
assets: ['index.js'],
},
]);

let dedicated, shared;
b.traverseBundles((bundle, ctx, traversal) => {
if (bundle.getMainEntry()?.filePath.endsWith('shared-worker.js')) {
shared = bundle;
} else if (
bundle.getMainEntry()?.filePath.endsWith('dedicated-worker.js')
) {
dedicated = bundle;
}
if (dedicated && shared) traversal.stop();
});

assert(dedicated);
assert(shared);

let main = await outputFS.readFile(b.getBundles()[0].filePath, 'utf8');
assert(/new Worker([^,]*?)/.test(main));
assert(/new SharedWorker([^,]*?)/.test(main));

dedicated = await outputFS.readFile(dedicated.filePath, 'utf8');
shared = await outputFS.readFile(shared.filePath, 'utf8');
let importRegex = supported ? /importScripts\s*\(/ : /import\s*("|')/;
assert(!importRegex.test(dedicated));
assert(!importRegex.test(shared));
});
}

it('should preserve the name option to workers', async function() {
let b = await bundle(
path.join(__dirname, '/integration/workers-module/named.js'),
Expand Down
1 change: 1 addition & 0 deletions packages/core/types/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,7 @@ export type InitialParcelOptions = {|
+publicUrl?: string,
+distDir?: FilePath,
+engines?: Engines,
+outputFormat?: OutputFormat,
|},

+additionalReporters?: Array<{|
Expand Down
11 changes: 7 additions & 4 deletions packages/transformers/js/src/JSTransformer.js
Original file line number Diff line number Diff line change
Expand Up @@ -484,15 +484,18 @@ export default (new Transformer({
for (let dep of dependencies) {
if (dep.kind === 'WebWorker') {
// Use native ES module output if the worker was created with `type: 'module'` and all targets
// support native module workers. Only do this if the source type is changing from script to module
// though so that assets can be shared between workers and the main thread in the global output format.
let outputFormat = asset.env.outputFormat;
// support native module workers. Only do this if parent asset output format is also esmodule so that
// assets can be shared between workers and the main thread in the global output format.
let outputFormat;
if (
asset.env.sourceType === 'script' &&
asset.env.outputFormat === 'esmodule' &&
Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't figure out why the source type has an influence on how assets can be shared. This really only depends on the output format?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think I probably just typed the wrong one... 😬

dep.source_type === 'Module' &&
supportsModuleWorkers
) {
outputFormat = 'esmodule';
} else {
outputFormat =
asset.env.outputFormat === 'commonjs' ? 'commonjs' : 'global';
}

let loc = convertLoc(dep.loc);
Expand Down