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.

Sign up for GitHub

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

Jump to bottom

esm: remove erroneous context.parentURL property passed to load hook #41975

Merged
merged 4 commits into from Feb 18, 2022
Merged

esm: remove erroneous context.parentURL property passed to load hook #41975

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5a9bf22
esm: remove erroneous `context.parentURL` property passed to `load` hook
JakobJingleheimer Feb 14, 2022
f733c70
fixup: ensure custom hooks are called & clarify import name
JakobJingleheimer Feb 15, 2022
b008799
fixup: add comments explaining purpose of new test set
JakobJingleheimer Feb 16, 2022
f39a2ca
fixup: avoid undesirably bypassing implementation
JakobJingleheimer Feb 17, 2022
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
1 change: 0 additions & 1 deletion lib/internal/modules/esm/loader.js
View file
Open in desktop
Expand Up @@ -282,7 +282,6 @@ class ESMLoader {
} = await this.load(url, {
format,
importAssertions,
parentURL,
});

const translator = translators.get(finalFormat);
Expand Down
70 changes: 70 additions & 0 deletions test/es-module/test-esm-loader-hooks.mjs
View file
Open in desktop
@@ -0,0 +1,70 @@
// Flags: --expose-internals
import '../common/index.mjs';
import mod from 'internal/modules/esm/loader';
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't find mod very descriptive (if it's a well known acronym that I don't know, please ignore)

Suggested change
import mod from 'internal/modules/esm/loader';
import loader from 'internal/modules/esm/loader';

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's not super common. I usually use module, but some linters freak out when they find it in an ESM file. I considered loader, but it's not the loader, so I opted for mod

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 went with esmLoaderModule (not married to it).

Copy link
Member

Choose a reason for hiding this comment

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

I dislike default exports in general. As far as I’m concerned, esm/loader doesn’t need a default export and you should be able to just import it via import { loader } from 'internal/modules/esm/loader'.

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, that's what I wanted to do. It doesn't work: #41975 (comment)

This was kind of weird btw: esm/loader.js exports ESMLoader as a prop on exports

node/lib/internal/modules/esm/loader.js

Line 589 in bbd7cf7

exports.ESMLoader = ESMLoader;

So I expect it to be exposed as a named export (thus precluding this altogether).

esmLoaderModule is an object that contains a property named ESMLoader; there are no named exports available :(

import assert from 'assert';

const { ESMLoader } = mod;

{
Copy link
Member

Choose a reason for hiding this comment

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

Why this big block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😅 #41975 (review)

Copy link
Member

Choose a reason for hiding this comment

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

Okay, but until you have the other cases it doesn’t make much sense to have just one block. I would also argue that perhaps other cases should be in new files, as this one’s already pretty large.

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 was thinking it would make sense to keep them in the same file. There are already a lot of files in this directory, many of which SEEM redundant (which makes it difficult to figure out what might or should be where). If the file gets renamed at the same time as more cases are added, git will likely break the history.

I could see removing the block for now and add it back when the other cases are added.

I would prefer to keep them in the same file though, unless they differ a lot or become unruly. Could I persuade you to wait and see?

Copy link
Member

Choose a reason for hiding this comment

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

I’ve already approved, so you can do whatever. Maybe just add a comment at the top of the block saying this is here because you anticipate adding more blocks in the future, and what this block is meant to test.

const esmLoader = new ESMLoader();
Mesteery marked this conversation as resolved.
Show resolved Hide resolved

const originalSpecifier = 'foo/bar';
const importAssertions = { type: 'json' };
const parentURL = 'file:///entrypoint.js';
const resolvedURL = 'file:///foo/bar.js';
const suggestedFormat = 'test';

const customLoader1 = {
resolve(specifier, context, defaultResolve) {
JakobJingleheimer marked this conversation as resolved.
Show resolved Hide resolved
assert.strictEqual(specifier, originalSpecifier);
assert.deepStrictEqual(Object.keys(context), [
'conditions',
'importAssertions',
'parentURL',
]);
assert.ok(Array.isArray(context.conditions));
assert.strictEqual(context.importAssertions, importAssertions);
assert.strictEqual(context.parentURL, parentURL);
assert.strictEqual(typeof defaultResolve, 'function');

// This doesn't matter. just to avoid errors
return {
format: suggestedFormat,
url: resolvedURL,
};
},
load(resolvedURL, context, defaultLoad) {
assert.strictEqual(resolvedURL, resolvedURL);
assert.ok(new URL(resolvedURL));
assert.deepStrictEqual(Object.keys(context), [
'format',
'importAssertions',
]);
assert.strictEqual(context.format, suggestedFormat);
assert.strictEqual(context.importAssertions, importAssertions);
assert.strictEqual(typeof defaultLoad, 'function');

// This doesn't matter. just to avoid errors
return {
format: 'module',
source: '',
};
},
};

esmLoader.addCustomLoaders(customLoader1);

esmLoader.resolve(
originalSpecifier,
parentURL,
importAssertions,
);
esmLoader.load(
resolvedURL,
{
format: suggestedFormat,
importAssertions,
},
function mockDefaultLoad() {},
);
}