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

Merged
merged 1 commit into from
Mar 14, 2022
Merged
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
esm: improve typings and code coverage
PR-URL: #42305
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
  • Loading branch information
bmeck authored and aduh95 committed Mar 14, 2022
commit 68626dc4514b70614cd345797662ad8b48735863
4 changes: 4 additions & 0 deletions lib/internal/modules/esm/formats.js
Original file line number Diff line number Diff line change
@@ -29,6 +29,10 @@ if (experimentalWasmModules) {
extensionFormatMap['.wasm'] = legacyExtensionFormatMap['.wasm'] = 'wasm';
}

/**
* @param {string} mime
* @returns {string | null}
*/
function mimeToFormat(mime) {
if (
RegExpPrototypeTest(
25 changes: 25 additions & 0 deletions lib/internal/modules/esm/get_format.js
Original file line number Diff line number Diff line change
@@ -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)?,/,
@@ -41,6 +45,12 @@ function getDataProtocolModuleFormat(parsed) {
return mimeToFormat(mime);
}

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

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

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) ?
1 change: 1 addition & 0 deletions lib/internal/modules/esm/resolve.js
Original file line number Diff line number Diff line change
@@ -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
Member

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;
7 changes: 5 additions & 2 deletions test/es-module/test-esm-basic-imports.mjs
Original file line number Diff line number Diff line change
@@ -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);