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 2 commits
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion lib/internal/modules/esm/loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,6 @@ class ESMLoader {
} = await this.load(url, {
format,
importAssertions,
parentURL,
});

const translator = translators.get(finalFormat);
Expand Down
73 changes: 73 additions & 0 deletions test/es-module/test-esm-loader-hooks.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
// Flags: --expose-internals
import { mustCall } from '../common/index.mjs';
import esmLoaderModule from 'internal/modules/esm/loader';
import assert from 'assert';

const { ESMLoader } = esmLoaderModule;

{
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
Member 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
Member 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();

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

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

return {
format: suggestedFormat,
url: resolvedURL,
};
}

function load(resolvedURL, context, defaultLoad) {
assert.strictEqual(resolvedURL, resolvedURL);
assert.ok(new URL(resolvedURL));
assert.deepStrictEqual(Object.keys(context), [
'format',
'importAssertions',
]);
Comment on lines +46 to +49
Copy link
Member

Choose a reason for hiding this comment

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

Can you make it clear somewhere that what you’re testing in this file is the signatures of the hooks? Like maybe name the file something like test-esm-loader-hooks-signatures.mjs, and add some comments near these assertions that what we’re doing here is testing that the expected arguments/properties exist and that unexpected ones don’t exist?

And I assume that’s all you’re aiming to test in this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you make it clear somewhere that what you’re testing in this file is the signatures of the hooks? […] add some comments near these assertions that what we’re doing here is testing that the expected arguments/properties exist and that unexpected ones don’t exist?

Sure!

And I assume that’s all you’re aiming to test in this file?

No, I'm planning (in the near future) to add more tests for hooks themselves in this file (hence the generic file name). Those aren't relevant to this PR, so I didn't add them yet.

Copy link
Member

Choose a reason for hiding this comment

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

It’s much easier to debug failing tests when each test file is as close as possible to one test per file, since you can re-run a test file like ./node test/es-module/test-esm-loader-hooks.mjs etc. The loader hooks API is large enough that I can’t imagine testing all of it in one file; you could make a subfolder for it perhaps.

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: '',
};
}

const customLoader1 = {
resolve: mustCall(resolve),
load: mustCall(load),
};

esmLoader.addCustomLoaders(customLoader1);

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