NodeResolver: Reload the closest package.json to an asset if it's a package entry

[wbinnssmith]

Closes #7908.

This heuristic is enough to solve the example in #7908, but it wouldn't hold in the case that the package.json defines a main as "./in/some/nested-package/index.js" where "./in/some/nested-package/package.json" also exists. Updated to handle this case.

Test Plan: Added an integration test.

[parcel-benchmark]

Benchmark Results

Kitchen Sink 🚨

Timings

Description	Time	Difference
Cold	FAILED	-0.00ms
Cached	FAILED	-0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

React HackerNews ✅

Timings

Description	Time	Difference
Cold	10.59s	+127.00ms
Cached	516.00ms	+17.00ms

Cold Bundles

Bundle	Size	Difference	Time	Difference
dist/logo.c5bb83f1.png	246.00b	+0.00b	5.48s	+707.00ms ⚠️

Cached Bundles

No bundle changes detected.

AtlasKit Editor 🚨

Timings

Description	Time	Difference
Cold	FAILED	-0.00ms
Cached	FAILED	-0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

Three.js ✅

Timings

Description	Time	Difference
Cold	7.62s	+32.00ms
Cached	296.00ms	-10.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Click here to view a detailed benchmark overview.

[mischnic]

For future reference, this bug is just another version of #5975

[mischnic]

Also, instead or additionally adding a resolver unit test might be good

[mischnic]

This update should definitely be done inside loadAsFile, but I think that additional parameter is kind of odd...
An alternative would be using pkg.pkgDir !== path.basename(found) ? await this.readPackage() : pkg.

[wbinnssmith]

An alternative would be using pkg.pkgDir !== path.basename(found) ? await this.readPackage() : pkg.

This doesn't really capture whether the file was loaded as a package entry, which is afaik the only thing that could invalidate the pkg we had loaded prior. It would also cause unnecessary reloads for files in packages with additional directories that were not themselves packages, yeah?

[mischnic]

file was loaded as a package entry, which is afaik the only thing that could invalidate the pkg we had loaded prior

Right

It would also cause unnecessary reloads for files in packages with additional directories that were not themselves packages

Every folder can contain a package.json, but there is only one case where it wouldn't match. So there shouldn't be unnecessary reloads.
And that pkg.pkgDir !== path.basename(found) condition should cause less loads in cases where the package entry field doesn't refer to a parent or sub folder?

(And if we do want to keep that flag, maybe rename asPackageEntry to reloadPkg?)

[mischnic]

So some of these new invalidations are just duplicates, some make sense (e.g. for should resolve node builtin modules) but some are somehow just not needed, like this one?
node_modules/foo/package.json already exists and it doesn't need to watch above that

    it('should resolve a node_modules index.js', async function () {
        let resolved = await resolver.resolve({
        env: BROWSER_ENV,
        filename: 'foo',
        specifierType: 'esm',
        parent: path.join(rootDir, 'foo.js'),
      });
      assert.deepEqual(resolved, {
        filePath: path.join(rootDir, 'node_modules', 'foo', 'index.js'),
        sideEffects: undefined,
        query: undefined,
        invalidateOnFileCreate: [
          {
            fileName: 'package.json',
            aboveFilePath: path.join(rootDir, 'index'),
          },
          {
            fileName: 'package.json',
            aboveFilePath: path.join(rootDir, 'foo.js'),
          },
          {
            fileName: 'node_modules/foo',
            aboveFilePath: path.join(rootDir, 'foo.js'),
          },
+         {
+           fileName: 'package.json',
+           aboveFilePath: path.join(rootDir, 'node_modules/foo/index.js'),
+         },
        ],
        invalidateOnFileChange: [
          path.join(rootDir, 'package.json'),
          path.join(rootDir, 'node_modules', 'foo', 'package.json'),
        ],

[wbinnssmith]

This regressed the build time of a large app by about 3.4% (203s to 196s for a complete build), though it is arguably a correctness and compatibility issue with other bundlers.

[mischnic]

@devongovett Does aboveFilePath also stop the watching in parent folder after encountering node_modules (like loadConfig does)? Or is this fine?

[devongovett]

This regressed the build time of a large app by about 3.4%

Hmm that's unfortunate.