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

module: unflag resolve self #31002

Closed
wants to merge 2 commits into from
Closed

Conversation

@guybedford
Copy link
Contributor

guybedford commented Dec 17, 2019

Unflags the --experimental-resolve-self option, which allows packages to load their own "exports" definitions through an import to their own package name.

For CommonJS this approach is backwards-compatible because the own name resolution is only attempted after all other resolutions fail.

Backwards compatibility is now ensured by only supporting own-name resolution when "exports" in the package.json is set. When it is, this resolution applies before any node_modules lookup checks.

Opening now to discuss along with overall resolver stability concerns.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Dec 17, 2019

Copy link
Member

MylesBorins left a comment

RSLGTM if tests are green

@nodejs-github-bot

This comment has been minimized.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Dec 17, 2019

Should #31009 land before this does?

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Dec 18, 2019

That would probably be slightly preferable, although they could land in either order, so long as the bug fix lands before the unflagging is released.

@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Dec 23, 2019

I think we should land behavior changes before unflagging

@guybedford guybedford force-pushed the guybedford:unflag-resolve-self branch from e963247 to c190b53 Dec 29, 2019
@guybedford guybedford added blocked and removed blocked labels Dec 29, 2019
@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Dec 29, 2019

The behaviour changes have landed in 8a96d05, so this is in theory ready to go now.

@guybedford guybedford added pending and removed blocked labels Dec 29, 2019
src/module_wrap.cc Outdated Show resolved Hide resolved
src/module_wrap.cc Outdated Show resolved Hide resolved
src/module_wrap.cc Outdated Show resolved Hide resolved
@guybedford guybedford force-pushed the guybedford:unflag-resolve-self branch from c190b53 to 514a61d Dec 29, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 30, 2019

Codecov Report

Merging #31002 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #31002   +/-   ##
=======================================
  Coverage   97.33%   97.33%           
=======================================
  Files         189      189           
  Lines       63888    63888           
=======================================
  Hits        62184    62184           
  Misses       1704     1704

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a96d05...514a61d. Read the comment docs.

@Trott
Trott approved these changes Dec 30, 2019
Copy link
Member

Trott left a comment

LGTM if the modules team is good with it.

@jkrems
jkrems approved these changes Dec 30, 2019
Copy link
Contributor

jkrems left a comment

LGTM but I think there's an orphaned file that could be deleted.

test/es-module/test-esm-exports.mjs Show resolved Hide resolved
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment has been minimized.

Copy link

nodejs-github-bot commented Jan 2, 2020

@BridgeAR BridgeAR removed the pending label Jan 2, 2020
BridgeAR added a commit that referenced this pull request Jan 2, 2020
PR-URL: #31002
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Jan 2, 2020

Landed in c7f328f 🎉

@BridgeAR BridgeAR closed this Jan 2, 2020
BridgeAR added a commit that referenced this pull request Jan 3, 2020
PR-URL: #31002
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
MylesBorins added a commit that referenced this pull request Jan 12, 2020
PR-URL: #31002
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.