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

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

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
1 change: 0 additions & 1 deletion lib/internal/modules/esm/loader.js
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
76 changes: 76 additions & 0 deletions test/es-module/test-esm-loader-hooks.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
// Flags: --expose-internals
import { mustCall } from '../common/index.mjs';
import esmLoaderModule from 'internal/modules/esm/loader';
import assert from 'assert';

const { ESMLoader } = esmLoaderModule;

/**
* Verify custom hooks are called with appropriate arguments.
*/
{
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.

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 = Object.assign(
Object.create(null),
{ 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);
// Ensure `context` has all and only the properties it's supposed to
assert.deepStrictEqual(Object.keys(context), [
'conditions',
'importAssertions',
'parentURL',
]);
assert.ok(Array.isArray(context.conditions));
assert.deepStrictEqual(context.importAssertions, importAssertions);
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));
// Ensure `context` has all and only the properties it's supposed to
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.deepStrictEqual(context.importAssertions, importAssertions);
assert.strictEqual(typeof defaultLoad, 'function');

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

const customLoader = {
// Ensure ESMLoader actually calls the custom hooks
resolve: mustCall(resolve),
load: mustCall(load),
};

esmLoader.addCustomLoaders(customLoader);

// Manually trigger hooks (since ESMLoader is not actually running)
const job = await esmLoader.getModuleJob(
originalSpecifier,
parentURL,
importAssertions,
);
await job.modulePromise;
}