module: use undefined if no main #18593

Trott · 2018-02-06T06:07:28Z

If the package.json file does not have a "main" entry, return undefined
rather than an empty string. This is to make more consistent behavior.
For example, when package.json is a directory, "main" is undefined
rather than an empty string.

@bmeck @bnoordhuis Is this an acceptable resolution of the "package.json as a directory leaves main undefined if we land #18270, but package.json as a file without a main sets main to an empty string" disagreement? Opening a PR because I figure it would be easier to determine "yes" or "no" with an actual proposal to look at. I believe @bmeck would prefer the empty string for both scenarios, but I also believe that undefined is acceptable to them too as long as it applies to both scenarios. But I could be wrong...

Checklist

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

Affected core subsystem(s)

module

If the package.json file does not have a "main" entry, return undefined rather than an empty string. This is to make more consistent behavior. For example, when package.json is a directory, "main" is undefined rather than an empty string.

jdalton · 2018-02-06T08:38:22Z

Would this naturally shake out of @guybedford's package.json reading work for the "mode":"esm"?
If so, since there's no rush, that could be a way to deal with it.

bmeck · 2018-02-06T13:42:10Z

As long as they are the same that seems fine. I will still open up a PR to cache all the results unconditionally sometime in the future though.

Trott · 2018-02-06T17:03:35Z

I'm going to mark this as blocked because it should only land if #18270 lands first.

Trott · 2018-02-08T03:23:53Z

Landing #18270 as soon as I get green/yellow CI. Unblocking this. This could definitely use some more reviews. Ping @nodejs/collaborators

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

devsnek · 2018-02-08T03:26:23Z

seems like much saner behavior, lgtm

Trott · 2018-02-08T04:46:00Z

Ci is yellow. 🎉

Trott · 2018-02-08T05:08:57Z

This is semver-major so it needs two approvals from @nodejs/tsc before it can land. Please review! Thanks!

Trott · 2018-02-08T06:08:18Z

Also needs a CITGM run: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1266/

jdalton · 2018-02-08T06:08:56Z

Just for context this change is sitting on top of other semver-major changes like InternalModuleReadJSON.

Trott · 2018-02-08T06:17:13Z

Just for context this change is sitting on top of other semver-major changes like InternalModuleReadJSON.

I only did a cursory look at first and saw this change affects the results of tests that are already in v9.x-staging. So it semver-major! But now I see that it may only change internal behavior and may not be observable by the end user without dipping into process.binding('fs'). So maybe not semver-major after all? In any event, I don't mind being cautious. I will certainly feel differently if this stalls, though. :-D

benjamingr · 2018-02-08T08:03:06Z

@nodejs/modules

hiroppy

👍

Trott · 2018-02-09T21:27:21Z

Still needs one more @nodejs/tsc approval.

Fishrock123 · 2018-02-10T17:13:42Z

I don't have any strong feelings about this but would like to hear @bnoordhuis's opinion, if he has one.

Trott · 2018-02-11T01:07:13Z

Can someone familiar with CITGM review the CITGM results or point me to documentation that will explain how to review result? https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1266/ I looked over them but I'm not sure how to tell what's expected failures and what's not. @nodejs/citgm

mcollina · 2018-02-11T07:54:57Z

@Trott that looks ok to me. I'm currently working on fixing some of those packages.

mcollina

LGTM

If the package.json file does not have a "main" entry, return undefined rather than an empty string. This is to make more consistent behavior. For example, when package.json is a directory, "main" is undefined rather than an empty string. PR-URL: nodejs#18593 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Trott · 2018-02-11T22:29:13Z

Landed in bd4773a.

bnoordhuis · 2018-02-12T13:51:25Z

I don't have any strong feelings about this but would like to hear @bnoordhuis's opinion, if he has one.

Belated: this PR should have removed the if (json === '') check from lib/module.js, it's dead code now.

More importantly, it undoes the optimization that a package.json without a 'main' property isn't loaded from disk repeatedly.

That said, the logic in readPackage() looks kind of broken anyway. It's quite possibly no worse than before.

Trott · 2018-02-13T22:47:43Z

Belated: this PR should have removed the if (json === '') check from lib/module.js, it's dead code now.

Thanks. That can be fixed in a subsequent PR. I'll submit it shortly if no one else has done so already.

More importantly, it undoes the optimization that a package.json without a 'main' property isn't loaded from disk repeatedly.

If I understand correctly (which is a pretty big caveat), that would be fixed by caching negative results, which @bmeck intends to introduce. Not sure if that is likely to get much push-back or not.

bmeck · 2018-02-13T23:01:52Z

@Trott pretty much, but I've been talking to @guybedford about some stuff we can do to make the cache a bit better and moved to C++ so that it can be used prior to crossing the C++/JS bridge and same as #18728 . Might be a little bit pending that.

Remove unnecessary condition in lib/module.js. Refs: nodejs#18593 (comment)

Remove unnecessary condition in lib/module.js. Refs: nodejs#18593 (comment) PR-URL: nodejs#18768 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

If the package.json file does not have a "main" entry, return undefined rather than an empty string. This is to make more consistent behavior. For example, when package.json is a directory, "main" is undefined rather than an empty string. PR-URL: nodejs#18593 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Remove unnecessary condition in lib/module.js. Refs: nodejs#18593 (comment) PR-URL: nodejs#18768 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

module: use undefined if no main

d143e42

If the package.json file does not have a "main" entry, return undefined rather than an empty string. This is to make more consistent behavior. For example, when package.json is a directory, "main" is undefined rather than an empty string.

Trott added module Issues and PRs related to the module subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Feb 6, 2018

Trott requested review from bmeck and bnoordhuis February 6, 2018 06:07

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. labels Feb 6, 2018

bmeck approved these changes Feb 6, 2018

View reviewed changes

Trott added the blocked PRs that are blocked by other issues or PRs. label Feb 6, 2018

This was referenced Feb 7, 2018

Node.js Foundation Technical Steering Committee (TSC) Meeting 2018-02-07 nodejs/TSC#484

Closed

src: don't abort when package.json is a directory #18270

Closed

Trott removed the blocked PRs that are blocked by other issues or PRs. label Feb 8, 2018

targos approved these changes Feb 8, 2018

View reviewed changes

benjamingr approved these changes Feb 8, 2018

View reviewed changes

vdeturckheim approved these changes Feb 8, 2018

View reviewed changes

hiroppy approved these changes Feb 8, 2018

View reviewed changes

ljharb approved these changes Feb 9, 2018

View reviewed changes

bmeurer approved these changes Feb 9, 2018

View reviewed changes

ofrobots approved these changes Feb 9, 2018

View reviewed changes

mcollina approved these changes Feb 11, 2018

View reviewed changes

Trott closed this Feb 11, 2018

Trott added a commit to Trott/io.js that referenced this pull request Feb 13, 2018

module: remove unused code in module.js

dca2148

Remove unnecessary condition in lib/module.js. Refs: nodejs#18593 (comment)

Trott mentioned this pull request Feb 13, 2018

module: remove unused code in module.js #18768

Closed

2 tasks

Trott deleted the empty-string-undefined branch January 13, 2022 22:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: use undefined if no main #18593

module: use undefined if no main #18593

Trott commented Feb 6, 2018

jdalton commented Feb 6, 2018 •

edited

Loading

bmeck commented Feb 6, 2018

Trott commented Feb 6, 2018

Trott commented Feb 8, 2018

devsnek commented Feb 8, 2018

Trott commented Feb 8, 2018

Trott commented Feb 8, 2018

Trott commented Feb 8, 2018

jdalton commented Feb 8, 2018

Trott commented Feb 8, 2018

benjamingr commented Feb 8, 2018

hiroppy left a comment

Trott commented Feb 9, 2018

Fishrock123 commented Feb 10, 2018

Trott commented Feb 11, 2018

mcollina commented Feb 11, 2018

mcollina left a comment

Trott commented Feb 11, 2018

bnoordhuis commented Feb 12, 2018

Trott commented Feb 13, 2018

bmeck commented Feb 13, 2018

module: use undefined if no main #18593

module: use undefined if no main #18593

Conversation

Trott commented Feb 6, 2018

Checklist

Affected core subsystem(s)

jdalton commented Feb 6, 2018 • edited Loading

bmeck commented Feb 6, 2018

Trott commented Feb 6, 2018

Trott commented Feb 8, 2018

devsnek commented Feb 8, 2018

Trott commented Feb 8, 2018

Trott commented Feb 8, 2018

Trott commented Feb 8, 2018

jdalton commented Feb 8, 2018

Trott commented Feb 8, 2018

benjamingr commented Feb 8, 2018

hiroppy left a comment

Choose a reason for hiding this comment

Trott commented Feb 9, 2018

Fishrock123 commented Feb 10, 2018

Trott commented Feb 11, 2018

mcollina commented Feb 11, 2018

mcollina left a comment

Choose a reason for hiding this comment

Trott commented Feb 11, 2018

bnoordhuis commented Feb 12, 2018

Trott commented Feb 13, 2018

bmeck commented Feb 13, 2018

jdalton commented Feb 6, 2018 •

edited

Loading