fix: use require.resolve when resolving default config

[paul-soporan]

↪️ Pull Request

2.0.0-nightly.396 regressed PnP support in #4927, by not using require.resolve to load the defaultConfig - @parcel/config-default: https://github.com/parcel-bundler/parcel/pull/4927/files#diff-57d3138ed35ffa967c184c701e6efea4R181.

Yarn's E2E tests caught this issue after 2.0.0-nightly.396 was released: https://github.com/yarnpkg/berry/runs/1097463270?check_suite_focus=true#step:5:157.

This was solved by using require.resolve to resolve @parcel/config-default, which causes it to be resolved on behalf of parcel. I can confirm that this change fixes the issue.

💻 Examples

cd $(mktemp -d)
yarn init -2y
yarn add parcel@2.0.0-nightly.396 @babel/core
echo "console.log('Hello World')" > index.js
yarn parcel build index.js

Expected: Build should succeed.
Got: Error: Cannot find module '@parcel/config-default'

🚨 Test instructions

Parcel's PnP test didn't catch this because it goes through @parcel/test-utils instead of the CLI. I've looked a bit into Parcel's testing infrastructure, but I couldn't find tests testing the actual CLI (In case they exist, I'd be happy to add a basic PnP CLI test).

✔️ PR Todo

• Added/updated unit tests for this change
• Filled out test instructions (In case there aren't any unit tests)
• Included links to related issues/PRs

[height]

Link Height tasks by mentioning a task ID in the pull request title or description, commit messages, or comments.

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

[wbinnssmith]

cc @padmaia

[mischnic]

I think we actually want to try packageManager.resolve first and then fallback to require.resolve so that we use a user-installed version of @parcel/config-default if possible and otherwise fall back?

[paul-soporan]

Are there any advantages to using packageManager.resolve instead of require.resolve('@parcel/config-default', { paths: [fs.cwd(), __dirname] }) (which also achieves the fallback without a try / catch)?

[mischnic]

Generally, packageManger would also read from a potential MemoryFS as inputFS. Though that will never be the case with the CLI

I didn't know about the paths option 😄 👍

[mischnic]

Apparently even Flow doesn't know about that option:

$ /home/vsts/work/1/s/node_modules/.bin/flow check
Error --------------------------------------------------------------------------- packages/core/parcel/src/cli.js:182:62

Cannot call `require.resolve` because no more than 1 argument is expected by function type [1]. [extra-arg]

   packages/core/parcel/src/cli.js:182:62
   182|     defaultConfig: require.resolve('@parcel/config-default', { paths: [fs.cwd(), __dirname] }),
                                                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

References:
   /tmp/flow/flowlib_39556f04/core.js:901:14
   901|     resolve: (id: string) => string,
                     ^^^^^^^^^^^^^^^^^^^^^^ [1]

Can you add a // $FlowFixMe comment above that call?

[paul-soporan]

Done! Apparently the Flow server in VSCode decided to take a nap while I was developing. 😅

[mischnic]

Waiting for @padmaia to double check...