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: fix main lookup regression from #18728 #18788

Closed
wants to merge 1 commit into from

Conversation

guybedford
Copy link
Contributor

There was a minor regression in #18728 in the package.json main resolution code path.

This includes a test and a fix, hopefully these sort of slips will become harder as the test coverage on the modules work increases.

@bnoordhuis one of the slips here was on the enum booleans being checked as booleans. I considered making them extend boolean, but I assumed the intention is that usage should always be explicit with these, although it's a shame this can't be caught by the compiler at all.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

modules

@guybedford guybedford added the esm Issues and PRs related to the ECMAScript Modules implementation. label Feb 14, 2018
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 14, 2018
@guybedford guybedford force-pushed the main-lookup-regression branch from b419326 to 3e9066c Compare February 14, 2018 21:46
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

This can be fast tracked IMO

@benjamingr benjamingr added fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 15, 2018
@benjamingr
Copy link
Member

Also, that's weird @guybedford - it shouldn't be possible as far as I know :O http://coliru.stacked-crooked.com/a/efc7c335a197e56d

@jdalton
Copy link
Member

jdalton commented Feb 15, 2018

@guybedford What was the regression?

@guybedford
Copy link
Contributor Author

@jdalton the process of applying the package.json main stopped working.

@benjamingr that is really odd then that there was not even a compiler warning, and seems like it could be a problem!

Since we don't have compiler warnings for these cases, do you think I should just extend bool for these enums and switch their parity so they can be treated as booleans without breaking? Just thinking of maintainability here.

@guybedford
Copy link
Contributor Author

@BridgeAR
Copy link
Member

Landed in 7748865 🎉

@BridgeAR BridgeAR closed this Feb 16, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 16, 2018
PR-URL: nodejs#18788
Refs: nodejs#18728
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@guybedford guybedford deleted the main-lookup-regression branch February 17, 2018 09:47
@MylesBorins
Copy link
Contributor

this should be backported with #18728

guybedford added a commit to guybedford/node that referenced this pull request Mar 4, 2018
PR-URL: nodejs#18788
Refs: nodejs#18728
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 7, 2018
Backport-PR-URL: #18923
PR-URL: #18788
Refs: #18728
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 7, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
PR-URL: nodejs#18788
Refs: nodejs#18728
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants