fix(utils): Fix loadModule when using PnP

[andreialecu]

Fixes #4076

Uses createRequire to ensure that modules are correctly located and scoped to the main package that is running.

See:
https://nodejs.org/docs/latest/api/modules.html#modules_accessing_the_main_module
https://nodejs.org/api/module.html#module_module_createrequire_filename

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

• If you've added code that should be tested, please add tests.
• Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

[andreialecu]

Note that this should also be an improvement for a normal node_modules based installation, by ensuring that the modules are looked for relative to the entry point of the application.

There could be cases where the current code fails to require the proper module, depending on how things are hoisted. For more details expand the "Why?" paragraph here: https://yarnpkg.com/advanced/rulebook/#packages-should-only-ever-require-what-they-formally-list-in-their-dependencies

Also, it falls back to pre-existing implementation if this fails.

[andreialecu]

Also note that this requires node 12, but yarn 2+ itself requires node 12+.

And it should fall back otherwise via the empty catch.

[andreialecu]

@kamilogorek I noticed you added this function in #3483

If require.resolve was able to be used then this PR wouldn't be necessary.

Care to explain with an example why this loadModule function is useful?

This PR does make things work but it's possible that the function could be rewritten and the overall implementation might be able to be simplified.

Here's a super simple implementation as a replacement for loadModule that also fixes the problem in my project:

function loadModule(moduleName) {
    try {
        return dynamicRequire(require.main, moduleName);
    }
    catch (e) {
        return undefined;
    }
}

Notice the usage of require.main.

Any thoughts on it?

[kamilogorek]

Care to explain with an example why this loadModule function is useful?

I left a large comment there in case we need to understand this again. Let me know if it's not clear enough though.

 * Helper for dynamically loading module that should work with linked dependencies.
 * The problem is that we _should_ be using `require(require.resolve(moduleName, { paths: [cwd()] }))`
 * However it's _not possible_ to do that with Webpack, as it has to know all the dependencies during
 * build time. `require.resolve` is also not available in any other way, so we cannot create,
 * a fake helper like we do with `dynamicRequire`.
 *
 * We always prefer to use local package, thus the value is not returned early from each `try/catch` block.
 * That is to mimic the behavior of `require.resolve` exactly.

[andreialecu]

I read the comment, but I guess I need to understand it with an example.

Or rather, would the implementation in my comment above fit the requirements? Because it is easier to understand, and it fixes the PnP problem.

[kamilogorek]

The main reason is that you cannot use linked packages without process based require and require.resolve doesn't work with webpack, because it will be optimized-out of the bundle - same goes for any require that's performed at runtime.

We had multiple issues with it in the past, and every change adds additional risk without lots of tests, see:
#2971
#2515
#3293

And all the comments in related issues.

---

Without process-based require:

With process-based required:

[andreialecu]

@kamilogorek can you try the code I suggested in that comment?

function loadModule(moduleName) {
    try {
        return dynamicRequire(require.main, moduleName);
    }
    catch (e) {
        return undefined;
    }
}

Notice the require.main to get the main module instead of using module.

The problem with hardcoding node_modules is that it does not exist for things like Yarn versions 2 and beyond in PnP mode. Also see: https://yarnpkg.com/advanced/rulebook/#modules-shouldnt-hardcode-node_modules-paths-to-access-other-modules

[kamilogorek]

It does appear to work, however, I still need to test it with auto-loading of db integrations (will do it today/tomorrow) and get back to you.
Would this change in itself (from the comment above), fix your original issue from #4076?

edit: It appears to be working fine, and we could swap the implementation. cc @lobsterkatie @AbhiPrasad @iker-barriocanal

[iker-barriocanal]

LGTM. What about adding a comment about why that block is added? Without context, it's hard to understand the reason for it being there.

[andreialecu]

Would this change in itself (from the comment above), fix your original issue from #4076?

Yes, it does fix it.

[andreialecu]

@iker-barriocanal I updated the comment and the implementation. Please double check that it passes all your requirements as per this last commit before merging.

[iker-barriocanal]

LGTM

[github-actions]

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

[iker-barriocanal]

@AbhiPrasad @lobsterkatie is there any reason to not merge this? It's been a while and it seems to work fine. If there are no reasons, I'll merge it.

[AbhiPrasad]

I'm fine with merging, I think the worry is that we haven't tested all the use cases out yet. Maybe we need to add some tests alongside this function change to get more confidence.