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

[v9.x backport] module: main w/o extension, pjson cache (#18728, #18788) #18923

Closed
wants to merge 2 commits into from
Closed

[v9.x backport] module: main w/o extension, pjson cache (#18728, #18788) #18923

wants to merge 2 commits into from

Conversation

guybedford
Copy link
Contributor

This includes the v9 backports for #18728 and #18788.

The conflict resolution is only in the first commit for #18728 here.

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

@guybedford guybedford added the addons Issues and PRs related to native addons. label Feb 21, 2018
@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. v9.x labels Feb 21, 2018
@guybedford guybedford mentioned this pull request Feb 21, 2018
Closed
4 tasks
@guybedford guybedford closed this Feb 25, 2018
@guybedford
Copy link
Contributor Author

I'd like to reopen this for consideration against #19040.

@guybedford guybedford reopened this Feb 27, 2018
@guybedford guybedford changed the base branch from v9.x-staging to v9.x February 27, 2018 21:02
@guybedford
Copy link
Contributor Author

CI: https://ci.nodejs.org/job/node-test-pull-request/13400/

@guybedford guybedford changed the title [v9.x] module: main w/o extension, pjson cache (#18728, #18788) [v9.x backport] module: main w/o extension, pjson cache (#18728, #18788) Feb 27, 2018
@rvagg
Copy link
Member

rvagg commented Mar 1, 2018

@MylesBorins re your comments in our TSC meeting about modules work going on, I'm guessing that we may want to hold off on pushing this kind of change out via Current just now until the modules group gets a chance to get its footing and make some more comprehensive decisions about ESM?

@guybedford
Copy link
Contributor Author

@rvagg thanks for checking on this one. It does fix a backwards compatibility concern so that's the main reason I'm pushing it. This is relatively non-controversial so shouldn't be too much of an issue I'd expect, but will be interesting to hear what Myles thinks as well.

The main blocker at the moment though is that CI is failing and I haven't had a moment to look into this further...! If I can't get to the fix in time, no problem at all to skip this for now.

@guybedford guybedford closed this Mar 4, 2018
@guybedford guybedford reopened this Mar 4, 2018
@guybedford
module: support main w/o extension, pjson cache
05b9f69 
This adds support for ensuring that the top-level main into Node is
supported loading when it has no extension for backwards-compat with
NodeJS bin workflows.

In addition package.json caching is implemented in the module lookup
process.

PR-URL: nodejs#18728
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@guybedford
module: fix main lookup regression from nodejs#18728
a366599 
PR-URL: nodejs#18788
Refs: nodejs#18728
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@guybedford
Copy link
Contributor Author

I finally fixed the issue here.

CI: https://ci.nodejs.org/job/node-test-pull-request/13485/

@MylesBorins MylesBorins mentioned this pull request Mar 6, 2018
Closed
MylesBorins pushed a commit that referenced this pull request Mar 7, 2018
@guybedford @MylesBorins
module: support main w/o extension, pjson cache
f3e3429 
This adds support for ensuring that the top-level main into Node is
supported loading when it has no extension for backwards-compat with
NodeJS bin workflows.

In addition package.json caching is implemented in the module lookup
process.

Backport-PR-URL: #18923
PR-URL: #18728
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 7, 2018
@guybedford @MylesBorins
module: fix main lookup regression from #18728
39e032f 
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
Copy link
Contributor

landed in 50d1233...39e032f

@MylesBorins MylesBorins closed this Mar 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
addons Issues and PRs related to native addons. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@guybedford @rvagg @MylesBorins @nodejs-github-bot