src: don't abort when package.json is a directory by bnoordhuis · Pull Request #18270 · nodejs/node

merging to my PR. would like to see more cooperation towards mentorship in future.

@bnoordhuis I disagree on landing it, but would like to know if your block required the difference in our opinions.

LGTM as this is.

Given that I’d expect directories called package.json to almost never show up in the real world, I’m okay with tending towards semantical correctness and not entering the entry into the cache.

(I’d also be okay with the other approach.)

@addaleax falsey values put into the cache don't even work today, hence my need of rereview of the "parent" PR. I still think semantics are closer to a file with empty content than a permissions error since we can inspect the entity on disk.

@bmeck I disagree with your last statement.

Also, is there any practical difference?

@addaleax

I disagree with your last statement.

Then we are at an impasse. I feel rather strongly one way, and others feel another way.

Also, is there any practical difference?

If it gets put into the cache or not, so C++ calls back into the function / stats. Likelyhood of this situation occurring is low but seems relevant still. Of note we also return "" for packages without main fields at all rather than treat it as an error.

would like to know if your block required the difference in our opinions

I don't understand the question. What block?

@bnoordhuis

so I can move forward with work that depends on it.

Still not sure I'm following... the work I'm referring to is unrelated to the module system - I want to factor out file reading in C++.

So it can move forward without this PR?

No, because I want to repurpose code that this PR touches.

@bnoordhuis

If it is not dependent on the module system, but needs some code this PR touches, Would your refactor be any different, due to making the InternalModuleReadJSON return "" as I requested above?

As an alternative I'd also be fine removing "" from being returned on missing "main" in package.json, since it would never be hit by the cache check anyway without a fix.

The consistency between the two is my real issue with not returning "" so as long as it is one way or another.

I guess both PRs can go to tsc-review if we are stuck as well.

This seems good to me, the function is returning the package.json contents, so an empty return corresponding to no package.json file seems sensible in terms of the function's operation.

That said, the lack of a package.json can be cached just like the package.json itself, which doesn't currently seem to be the case. Having this scenario register into the cache may be a worthwhile followup PR especially with the ESM loader package.json checks which we should be getting to share this cache as well.

Cache of all package.json situations seems fine to me, but my concern is we don't have a clear definition with what it means to be an error. We have:

1. some error behavior that doesn't get cached for things like ENOENT and EPERM
2. some defaulting behavior with the missing "main" string in the file contents in which we attempt to cache as a permanent "error"?
   • it doesn't work since falsey values are not hits in the cache
   • this behavior gets weird, things like {"name": "main"} avoid this check.
3. we don't cache JSON.parse errors, and we even throw on them
4. we don't actually check if the JSON itself has a "main" property using getOwnPropertyDescriptor or anything, but we do cache the result.
   • you can have odd situations here as well with things like 0 as the package.json contents.

I'm trying to figure out how to make this more internally consistent:

1 doesn't cache errors, but doesn't treat it as having a value.
2 caches the error with a default value.
3 doesn't cache errors, but does throw.
4 just seems like a bug

I would like to choose one of 1, 2, 3, or caching the error and throwing as a consistent behavior with a personal preference on always caching the results. I think this preference is from working with ESM to a small extent that by spec caches errors permanently when importing other modules.

@bmeck Can you retract your red X? It's fine if you want to discuss semantics but please open a new issue. It shouldn't hold up a bug fix.

@bnoordhuis we have a disagreement, so no. Per stopping fixes, you also keep a red X on my PR. Your choice to open a new PR and not contribute to the original makes this rather hard as well. Maybe keeping things in one place would have been better, but now we have 2 PRs that neither of us likes apparently. One that does acknowledge both contributors involved and solved the review comments. You can remove your X on mine or take one of the 2 suggestions above we can land either PR.

In the future, I highly recommend a different approach to superceding PRs be looked into as your behavior of open a PR that only you commit to ontop of mine has happened a couple times and this scenario seems to be common that you give me a red X and tell me that I am blocking progress or have even insulted me in the past.

Well, okay then. @nodejs/tsc Please weigh in.

I think we should treat this the same as ENOENT here and make all of this more consistent in a subsequent PR.