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 new ESM loader runtime by moving to abs URLs #9172

Merged
merged 12 commits into from
Aug 4, 2023
25 changes: 21 additions & 4 deletions packages/core/core/src/applyRuntimes.js
Expand Up @@ -6,7 +6,6 @@ import type {SharedReference} from '@parcel/workers';
import type {
Asset,
AssetGroup,
Bundle,
Bundle as InternalBundle,
Config,
DevDepRequest,
Expand Down Expand Up @@ -41,6 +40,26 @@ type RuntimeConnection = {|
isEntry: ?boolean,
|};

function nameRuntimeBundle(
bundle: InternalBundle,
siblingBundle: InternalBundle,
) {
// We don't run custom namers on runtime bundles as the runtime assumes that they are
// located at the same nesting level as their owning bundle. Custom naming could
// be added in future as long as the custom name is validated.
let {hashReference} = bundle;

let name = nullthrows(siblingBundle.name)
// Remove the existing hash from standard file patterns
// e.g. 'main.[hash].js' -> 'main.js' or 'main~[hash].js' -> 'main.js'
.replace(new RegExp(`[\\.~\\-_]?${siblingBundle.hashReference}`), '')
// Ensure the file ends with 'runtime.[hash].js'
.replace(`.${bundle.type}`, `.runtime.${hashReference}.${bundle.type}`);

bundle.name = name;
bundle.displayName = name.replace(hashReference, '[hash]');
}

export default async function applyRuntimes<TResult>({
bundleGraph,
config,
Expand All @@ -51,7 +70,6 @@ export default async function applyRuntimes<TResult>({
previousDevDeps,
devDepRequests,
configs,
nameRuntimeBundle,
}: {|
bundleGraph: InternalBundleGraph,
config: ParcelConfig,
Expand All @@ -62,7 +80,6 @@ export default async function applyRuntimes<TResult>({
previousDevDeps: Map<string, string>,
devDepRequests: Map<string, DevDepRequest>,
configs: Map<string, Config>,
nameRuntimeBundle: (bundle: Bundle) => Promise<void>,
|}): Promise<Map<string, Asset>> {
let runtimes = await config.getRuntimes();
let connections: Array<RuntimeConnection> = [];
Expand Down Expand Up @@ -152,7 +169,7 @@ export default async function applyRuntimes<TResult>({
}
bundleGraph.createBundleReference(bundle, connectionBundle);

await nameRuntimeBundle(connectionBundle);
nameRuntimeBundle(connectionBundle, bundle);
}

connections.push({
Expand Down
2 changes: 0 additions & 2 deletions packages/core/core/src/requests/BundleGraphRequest.js
Expand Up @@ -411,8 +411,6 @@ class BundlerRunner {
previousDevDeps: this.previousDevDeps,
devDepRequests: this.devDepRequests,
configs: this.configs,
nameRuntimeBundle: bundle =>
this.nameBundle(namers, bundle, internalBundleGraph),
});

// Add dev deps for namers, AFTER running them to account for lazy require().
Expand Down
9 changes: 4 additions & 5 deletions packages/core/integration-tests/test/bundler.js
Expand Up @@ -280,22 +280,21 @@ describe('bundler', function () {
assets: ['b.html'],
},
{
assets: ['a.js', 'bundle-url.js', 'cacheLoader.js', 'js-loader.js'],
assets: ['a.js', 'cacheLoader.js', 'js-loader.js'],
},
{
assets: ['bundle-manifest.js'], // manifest bundle
assets: ['bundle-manifest.js', 'bundle-url.js'], // manifest bundle
},
{
assets: [
'b.js',
'bundle-url.js',
'cacheLoader.js',
'js-loader.js',
'esmodule-helpers.js',
],
},
{
assets: ['bundle-manifest.js'], // manifest bundle
assets: ['bundle-manifest.js', 'bundle-url.js'], // manifest bundle
},
{
assets: ['c.js'],
Expand All @@ -305,7 +304,7 @@ describe('bundler', function () {
let aManifestBundle = b
.getBundles()
.find(
bundle => !bundle.getMainEntry() && /a\.HASH_REF/.test(bundle.name),
bundle => !bundle.getMainEntry() && bundle.name.includes('runtime'),
);

let bBundles = b
Expand Down
8 changes: 5 additions & 3 deletions packages/core/integration-tests/test/javascript.js
Expand Up @@ -2095,7 +2095,10 @@ describe('javascript', function () {
it('should create a shared bundle between browser and worker contexts', async () => {
let b = await bundle(
path.join(__dirname, '/integration/html-shared-worker/index.html'),
{mode: 'production', defaultTargetOptions: {shouldScopeHoist: false}},
{
mode: 'production',
defaultTargetOptions: {shouldScopeHoist: false, shouldOptimize: false},
},
);

assertBundles(b, [
Expand All @@ -2109,11 +2112,10 @@ describe('javascript', function () {
'get-worker-url.js',
'lodash.js',
'esmodule-helpers.js',
'bundle-url.js',
],
},
{
assets: ['bundle-manifest.js'],
assets: ['bundle-manifest.js', 'bundle-url.js'],
},
{
assets: ['worker.js', 'lodash.js', 'esmodule-helpers.js'],
Expand Down
5 changes: 4 additions & 1 deletion packages/core/test-utils/src/utils.js
Expand Up @@ -968,7 +968,10 @@ export async function runESM(
): Promise<Array<{|[string]: mixed|}>> {
let id = instanceId++;
let cache = new Map();
function load(specifier, referrer, code = null) {
function load(inputSpecifier, referrer, code = null) {
// ESM can request bundles with an absolute URL. Normalize this to the baseDir.
let specifier = inputSpecifier.replace('http://localhost', baseDir);

if (path.isAbsolute(specifier) || specifier.startsWith('.')) {
let extname = path.extname(specifier);
if (extname && extname !== '.js' && extname !== '.mjs') {
Expand Down
73 changes: 28 additions & 45 deletions packages/runtimes/js/src/JSRuntime.js
Expand Up @@ -390,17 +390,7 @@ function getLoaderRuntime({
!needsDynamicImportPolyfill &&
shouldUseRuntimeManifest(bundle, options)
) {
let params = [JSON.stringify(to.publicId)];

let relativeBase = getRelativeBasePath(
relativeBundlePath(bundle, to, {leadingDotSlash: false}),
);

if (relativeBase) {
params.push(relativeBase);
}

loaderModules.push(`load(${params.join(',')})`);
loaderModules.push(`load(${JSON.stringify(to.publicId)})`);
needsEsmLoadPrelude = true;
continue;
}
Expand All @@ -425,10 +415,12 @@ function getLoaderRuntime({
continue;
}

let code = `require(${JSON.stringify(loader)})(${getAbsoluteUrlExpr(
relativePathExpr,
bundle,
)})`;
let absoluteUrlExpr = shouldUseRuntimeManifest(bundle, options)
? `require('./helpers/bundle-manifest').resolve(${JSON.stringify(
to.publicId,
)})`
: getAbsoluteUrlExpr(relativePathExpr, bundle);
let code = `require(${JSON.stringify(loader)})(${absoluteUrlExpr})`;

// In development, clear the require cache when an error occurs so the
// user can try again (e.g. after fixing a build error).
Expand Down Expand Up @@ -632,39 +624,42 @@ function getRegisterCode(
entryBundle: NamedBundle,
bundleGraph: BundleGraph<NamedBundle>,
): string {
let idToName = {};
let idToName = [];
bundleGraph.traverseBundles((bundle, _, actions) => {
if (bundle.bundleBehavior === 'inline') {
return;
}

idToName[bundle.publicId] = path.basename(nullthrows(bundle.name));
idToName.push([
bundle.publicId,
relativeBundlePath(entryBundle, nullthrows(bundle), {
leadingDotSlash: false,
}),
]);

if (bundle !== entryBundle && isNewContext(bundle, bundleGraph)) {
for (let referenced of bundleGraph.getReferencedBundles(bundle)) {
idToName[referenced.publicId] = path.basename(
nullthrows(referenced.name),
);
idToName.push([
referenced.publicId,
relativeBundlePath(entryBundle, nullthrows(referenced), {
leadingDotSlash: false,
}),
]);
}
// New contexts have their own manifests, so there's no need to continue.
actions.skipChildren();
}
}, entryBundle);

return (
"require('./helpers/bundle-manifest').register(JSON.parse(" +
JSON.stringify(JSON.stringify(idToName)) +
'));'
);
}
let baseUrl =
entryBundle.env.outputFormat === 'esmodule' &&
entryBundle.env.supports('import-meta-url')
? 'new __parcel__URL__("").toString()' // <-- this isn't ideal. We should use `import.meta.url` directly but it gets replaced currently
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@devongovett Thoughts on this? Seems reasonable that I should be able to use import.meta.url without it being replaced?

I could modify the transform to add a new special parcel symbol (e.g. __parcel_importMetaUrl__)? Or potentially add some asset meta data that prevents the stripping?

Copy link
Member

Choose a reason for hiding this comment

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

hmm yeah, I guess we could add a new __parcel thing, but does this work as is for now? if so I'm ok with leaving it as you have it as well.

: `require('./helpers/bundle-url').getBundleURL('${entryBundle.publicId}')`;

function getRelativeBasePath(relativePath: string) {
// Get the relative part of the path. This part is not in the manifest, only the basename is.
let relativeBase = path.posix.dirname(relativePath);
if (relativeBase === '.') {
return '';
}
return JSON.stringify(relativeBase + '/');
return `require('./helpers/bundle-manifest').register(${baseUrl},JSON.parse(${JSON.stringify(
JSON.stringify(idToName),
)}));`;
}

function getRelativePathExpr(
Expand All @@ -673,18 +668,6 @@ function getRelativePathExpr(
options: PluginOptions,
): string {
let relativePath = relativeBundlePath(from, to, {leadingDotSlash: false});
if (shouldUseRuntimeManifest(from, options)) {
let basePath = getRelativeBasePath(relativePath);
let resolvedPath = `require('./helpers/bundle-manifest').resolve(${JSON.stringify(
to.publicId,
)})`;

if (basePath) {
return `${basePath} + ${resolvedPath}`;
}
return resolvedPath;
}

let res = JSON.stringify(relativePath);
if (options.hmrOptions) {
res += ' + "?" + Date.now()';
Expand Down
7 changes: 2 additions & 5 deletions packages/runtimes/js/src/helpers/browser/esm-js-loader.js
@@ -1,9 +1,6 @@
function load(id, base) {
let bundleId = require('../bundle-manifest').resolve(id);
let request = base ? './' + base + bundleId : './' + bundleId;

function load(id) {
// eslint-disable-next-line no-undef
return __parcel__import__(request);
return __parcel__import__(require('../bundle-manifest').resolve(id));
}

module.exports = load;
13 changes: 6 additions & 7 deletions packages/runtimes/js/src/helpers/bundle-manifest.js
@@ -1,18 +1,17 @@
var mapping = {};
var mapping = new Map();

Copy link
Member

Choose a reason for hiding this comment

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

Are we ok to drop browsers that don't support Map? I think Parcel's output still worked in IE11 up until this point. Maybe it's ok, but wonder if it should be done in a major version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe Map is supported in IE11. This support coverage looks good enough to me. But I can remove if you want.
https://caniuse.com/mdn-javascript_builtins_map

function register(pairs) {
var keys = Object.keys(pairs);
for (var i = 0; i < keys.length; i++) {
mapping[keys[i]] = pairs[keys[i]];
function register(baseUrl, pairs) {
for (var i = 0; i < pairs.length; i++) {
mapping.set(pairs[i][0], {baseUrl, path: pairs[i][1]});
}
mattcompiles marked this conversation as resolved.
Show resolved Hide resolved
}

function resolve(id) {
var resolved = mapping[id];
var resolved = mapping.get(id);
if (resolved == null) {
throw new Error('Could not resolve bundle with id ' + id);
}
return resolved;
return new URL(resolved.path, resolved.baseUrl).toString();
}
Copy link
Member

Choose a reason for hiding this comment

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

This one definitely won't work in IE. Probably ok at this point? I guess we can't simply concatenate the strings? path could probably start with ../ or something...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah not ideal. We could just recommend a polyfill for URL if anyone has issues I guess.


module.exports.register = register;
Expand Down