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: improve typings and code coverage #42305

Merged
merged 1 commit into from Mar 14, 2022
Merged

esm: improve typings and code coverage #42305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions lib/internal/modules/esm/formats.js
View file
Open in desktop
Expand Up @@ -29,6 +29,10 @@ if (experimentalWasmModules) {
extensionFormatMap['.wasm'] = legacyExtensionFormatMap['.wasm'] = 'wasm';
}

/**
* @param {string} mime
* @returns {string | null}
*/
function mimeToFormat(mime) {
if (
RegExpPrototypeTest(
Expand Down
25 changes: 25 additions & 0 deletions lib/internal/modules/esm/get_format.js
View file
Open in desktop
Expand Up @@ -32,6 +32,10 @@ const protocolHandlers = ObjectAssign(ObjectCreate(null), {
'node:'() { return 'builtin'; },
});

/**
* @param {URL} parsed
* @returns {string | null}
*/
function getDataProtocolModuleFormat(parsed) {
const { 1: mime } = RegExpPrototypeExec(
/^([^/]+\/[^;,]+)(?:[^,]*?)(;base64)?,/,
Expand All @@ -41,6 +45,12 @@ function getDataProtocolModuleFormat(parsed) {
return mimeToFormat(mime);
}

/**
* @param {URL} url
* @param {{parentURL: string}} context
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: context exists in a bunch of places, so it would be better to create a typedef and then reference that. I'd be happy to do in a follow-up too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since it's Bradley code, I feel inclined to land it as is (if I can get at least another 👍 on #42305 (comment)), and let you make a follow up PR since this one as already several approvals.

Copy link
Member

Choose a reason for hiding this comment

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

Along these lines, and also to consider in the follow-up PR rather than here, shouldn’t we be using true JSDoc syntax rather than TypeScript inside brackets? In other words, instead of:

 * @param {{parentURL: string}} context

We would write:

 * @param {object} context
 * @param {string} context.parentURL

Besides being correct JSDoc (in my understanding) the latter lets us add descriptions to each property of the object, like parentURL in this case.

* @param {boolean} ignoreErrors
* @returns {string}
*/
function getFileProtocolModuleFormat(url, context, ignoreErrors) {
const ext = extname(url.pathname);
if (ext === '.js') {
Expand All @@ -59,6 +69,11 @@ function getFileProtocolModuleFormat(url, context, ignoreErrors) {
return getLegacyExtensionFormat(ext) ?? null;
}

/**
* @param {URL} url
* @param {{parentURL: string}} context
* @returns {Promise<string> | undefined} only works when enabled
*/
function getHttpProtocolModuleFormat(url, context) {
if (experimentalNetworkImports) {
return PromisePrototypeThen(
Expand All @@ -70,13 +85,23 @@ function getHttpProtocolModuleFormat(url, context) {
}
}

/**
* @param {URL | URL['href']} url
* @param {{parentURL: string}} context
* @returns {Promise<string> | string | undefined} only works when enabled
*/
function defaultGetFormatWithoutErrors(url, context) {
const parsed = new URL(url);
if (!ObjectPrototypeHasOwnProperty(protocolHandlers, parsed.protocol))
return null;
return protocolHandlers[parsed.protocol](parsed, context, true);
}

/**
* @param {URL | URL['href']} url
* @param {{parentURL: string}} context
* @returns {Promise<string> | string | undefined} only works when enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

only works when enabled

Copy-pasta?

*/
function defaultGetFormat(url, context) {
const parsed = new URL(url);
return ObjectPrototypeHasOwnProperty(protocolHandlers, parsed.protocol) ?
Expand Down
1 change: 1 addition & 0 deletions lib/internal/modules/esm/resolve.js
View file
Open in desktop
Expand Up @@ -79,6 +79,7 @@ const DEFAULT_CONDITIONS_SET = new SafeSet(DEFAULT_CONDITIONS);
* @typedef {string | string[] | Record<string, unknown>} Exports
* @typedef {'module' | 'commonjs'} PackageType
* @typedef {{
* pjsonPath: string,
Copy link
Contributor

Choose a reason for hiding this comment

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

pjson?

* exports?: ExportConfig;
* name?: string;
* main?: string;
Expand Down
7 changes: 5 additions & 2 deletions test/es-module/test-esm-basic-imports.mjs
View file
Open in desktop
@@ -1,7 +1,10 @@
import '../common/index.mjs';
import assert from 'assert';
import ok from '../fixtures/es-modules/test-esm-ok.mjs';
import okShebang from './test-esm-shebang.mjs';
import * as okShebangNs from './test-esm-shebang.mjs';
// encode the '.'
import * as okShebangPercentNs from './test-esm-shebang%2emjs';

assert(ok);
assert(okShebang);
assert(okShebangNs.default);
assert.strict.equal(okShebangNs, okShebangPercentNs);