Skip to content

esm: fix emit deprecation on legacy main resolve #48664

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 7 commits into from
Jul 11, 2023

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jul 5, 2023

#48325 introduced a bug where the deprecation is thrown for every resolve. This PR fixes that.

Refs: #48325

@aduh95 aduh95 requested a review from anonrig July 5, 2023 18:23
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Jul 5, 2023
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

probably should add a test case of an ESM "main" that omits the leading ./?

@anonrig anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jul 5, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 5, 2023
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 5, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 5, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@H4ad
Copy link
Member

H4ad commented Jul 6, 2023

I think this change can be simplified to:

/**
 * Legacy CommonJS main resolution:
 * 1. let M = pkg_url + (json main field)
 * 2. TRY(M, M.js, M.json, M.node)
 * 3. TRY(M/index.js, M/index.json, M/index.node)
 * 4. TRY(pkg_url/index.js, pkg_url/index.json, pkg_url/index.node)
 * 5. NOT_FOUND
 * @param {URL} packageJSONUrl
 * @param {PackageConfig} packageConfig
 * @param {string | URL | undefined} base
 * @returns {URL}
 */
function legacyMainResolve(packageJSONUrl, packageConfig, base) {
  const packageJsonUrlString = packageJSONUrl.href;

  if (typeof packageJsonUrlString !== 'string') {
    throw new ERR_INVALID_ARG_TYPE('packageJSONUrl', ['URL'], packageJSONUrl);
  }

  const baseStringified = isURL(base) ? base.href : base;

  const resolvedOption = FSLegacyMainResolve(packageJsonUrlString, packageConfig.main, baseStringified);

  const baseUrl = resolvedOption <= legacyMainResolveExtensionsIndexes.kResolvedByMainIndexNode ? `./${packageConfig.main}` : '';
  const resolvedUrl = new URL(baseUrl + legacyMainResolveExtensions[resolvedOption], packageJSONUrl);

+ if (resolvedOption !== legacyMainResolveExtensionsIndexes.kResolvedByMain)
  emitLegacyIndexDeprecation(resolvedUrl, packageJSONUrl, base, packageConfig.main);
  return resolvedUrl;
}

The deprecation should be emitted for every case except from kResolvedByMain, ref: anonrig@a2a8e31#diff-b4c24f634e3741e5ad9e8c29864a48f2bd284a9d66d8fed3d077ccee0c44087bL160-L163

@anonrig anonrig added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jul 11, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 11, 2023
@nodejs-github-bot nodejs-github-bot merged commit ffb1929 into nodejs:main Jul 11, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in ffb1929

@aduh95 aduh95 deleted the fix-dep0151 branch July 11, 2023 17:39
juanarbol pushed a commit that referenced this pull request Jul 13, 2023
PR-URL: #48664
Refs: #48325
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@juanarbol juanarbol mentioned this pull request Jul 13, 2023
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48664
Refs: nodejs#48325
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48664
Refs: nodejs#48325
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
ruyadorno pushed a commit that referenced this pull request Sep 11, 2023
PR-URL: #48664
Refs: #48325
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@ruyadorno ruyadorno mentioned this pull request Sep 11, 2023
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
PR-URL: #48664
Refs: #48325
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
ruyadorno pushed a commit that referenced this pull request Sep 17, 2023
PR-URL: #48664
Refs: #48325
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
H4ad pushed a commit to H4ad/node that referenced this pull request Dec 3, 2023
PR-URL: nodejs#48664
Refs: nodejs#48325
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>

Backport-PR-URL: nodejs#48664
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants