nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.86k stars 29.72k forks source link

Add an entry to the module loading paths dynamically #18229

Closed mcollina closed 4 years ago

mcollina commented 6 years ago

From a twitter conversation it came out that a surprisingly common pattern to create a "plugin" system is the following:

process.env.NODE_PATH = "your/path";
require("module").Module._initPaths();

Sources: https://github.com/search?l=JavaScript&q=module._initPaths&type=Code&utf8= https://stackoverflow.com/questions/21358994/node-js-programmatically-setting-node-path https://twitter.com/boriscoder/status/953900513006931968

I propose we deprecate Module._initPaths() and we introduce:

process.addGlobalModulePath('your/path')

Or something similar (bikeshedding is ok).

mcollina commented 6 years ago

@bmeck how does this affect ESM?

cc @MylesBorins @jasnell

bmeck commented 6 years ago

@mcollina the usage recommendation for a NODE_PATH replacement for ESM is described in https://github.com/nodejs/node-eps/blob/master/002-es-modules.md#4321-how-to-support-non-local-dependencies. This solves the problem in a way that is not deprecated (though if you do it at runtime it is somewhat discouraged) and works in both ESM and CJS for the intended use case.

I think we should avoid global runtime mutation of this as NODE_PATH was deprecated entirely and should not be used at all. Even with the plugin pattern above, the state is leaking into dependencies. If a runtime based determination of path is needed a per package hook could solve it, or the user could write a --loader to solve this issue.

I'm a firm -1 on adding the same API as a runtime mutation unless it has a declarative counterpart such as --loader, and prefer that it be scoped to a package not the entire process. We should not be encouraging runtime global mutation of the loaders in standard user code, that is problematic for compatibility if we are aiming to have similar workflows in ahead of time tools/service workers. Let me know if the declarative solution in the EP works, and let me know if using a loader doesn't.

mcollina commented 6 years ago

@bmeck I see in the eps that the plan is for NODE_PATH to not be supported with ESM. https://github.com/nodejs/node-eps/blob/master/002-es-modules.md#432-removal-of-non-local-dependencies. Is this true right now?

What is proposed in https://github.com/nodejs/node-eps/blob/master/002-es-modules.md#4321-how-to-support-non-local-dependencies would not work for the target use case.

I think that we should determine if runtime mutation of the search path for modules is something we want to actively support. If we do not want it, then we remove it. If we want it, we support it properly with something that could work for both CJS and ESM.

cc @nodejs/tsc.

bmeck commented 6 years ago

I think that we should determine if runtime mutation of the search path for modules is something we want to actively support. If we do not want it, then we remove it. If we want it, we support it properly with something that could work for both CJS and ESM.

I think with loaders and how node_modules works, we can't prevent people from doing it. We can just document ways that they can do it. Not exposing a JS API goes a long way to make the ways to do it limited and easier to iterate/document.

bmeck commented 6 years ago

@mcollina forgot, yes NODE_PATH is not supported in --experimental-modules, nor ~/.node_libraries, require.extensions, etc. Basically, if it was deprecated and/or problematic for browser story it was removed.

Trott commented 5 years ago

Should this remain open?

jasnell commented 4 years ago

Given the lack of response in nearly a year, I'd guess no. @mcollina, if this is still something you want, please reopen.

aral commented 2 years ago

To add a use case: this is preventing me from making a Node.js web server that has native support for Svelte without requiring project authors to also install Svelte in their projects in order to use it.

The server in question is run with:

nodekit <path to project to serve>

In the project, authors create .page files that contain a superset of Svelte code and are read in and parsed using an ES Module Loader at runtime for a buildless workflow.

Currently, every project has to import svelte as I cannot specify a version that is installed with the server as the one to use.

I’m only hit by this when I try to compile the server-rendered Svelte file as I use esbuild to compile the hydration scripts and esbuild has proper support for node paths so it works without issue.