Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable core packages #216

Closed
egormalyutin opened this issue Mar 24, 2020 · 24 comments
Closed

Disable core packages #216

egormalyutin opened this issue Mar 24, 2020 · 24 comments

Comments

@egormalyutin
Copy link

Hello. Can I disable all core packages support?

@egormalyutin
Copy link
Author

egormalyutin commented Mar 24, 2020

I already came up with hack like

if (require("resolve/lib/core.json")[pkg]) {
	pkg += "/"
}
console.log(resolve(pkg))

@ljharb
Copy link
Member

ljharb commented Mar 24, 2020

What's your use case?

@ljharb
Copy link
Member

ljharb commented Mar 24, 2020

also that check is very incorrect; use require('resolve/lib/is-core')(pkg), if anything.

@egormalyutin
Copy link
Author

@ljharb I just want to find modules installed in node_modules rather than core modules

@egormalyutin
Copy link
Author

@ljharb Actually I used Object.property.hasOwnProperty and everything (it's just a short example), but thanks anyway!

@ljharb
Copy link
Member

ljharb commented Mar 25, 2020

That’s not correct, though, because of the semver ranges. Use the isCore function.

You can do that by appending a slash, just like you can in node.

@egormalyutin
Copy link
Author

@ljharb Okay, but can I add core module to the list?

@ljharb
Copy link
Member

ljharb commented Mar 25, 2020

Not without doing hacky unsupported things. What's your use case?

@egormalyutin
Copy link
Author

@ljharb I'm trying to create a simple small module bundler.

Can I submit a PR with support of adding or removing core modules?

@ljharb
Copy link
Member

ljharb commented Mar 26, 2020

browserify already is a simple small module bundler, and already uses resolve with existing options to achieve the same goal. If you can't use an existing bundler, i'd suggest reading how they do it :-)

I don't think a PR is warranted here.

@egormalyutin
Copy link
Author

@ljharb I'm not trying to create a module bundled because I don't know how to use them, but because I want to understand more how they work

@arikon
Copy link

arikon commented May 31, 2020

@ljharb My case is:

  • I want to recursively copy all the dependencies, found in package.json
  • one of the dependency names is clashing with the core module (buffer in my case), and is installed in node_modules
  • resolve knows buffer as a core module, so it don't even try to find a module in node_modules, it just returns buffer
  • but I expected a full module path

@arikon
Copy link

arikon commented May 31, 2020

I think the fix could be isCore() check happening after loadNodeModulesSync() call here:
https://github.com/browserify/resolve/blob/master/lib/sync.js#L88-L93

@octogonz
Copy link

I think the fix could be isCore() check happening after loadNodeModulesSync() call here:
https://github.com/browserify/resolve/blob/master/lib/sync.js#L88-L93

@arikon Hrmm.... I just tried the following test with Node 10:

$ npm init
$ npm install buffer
$ node -e console.log(require.resolve('buffer'))

It resolves to "buffer" instead of "node_modules/buffer". In other words, Node.js system modules seem to take precedence over installed NPM packages with the same name. Thus perhaps resolve.sync() is handling this case accurately.

Which package did you find that depends on "buffer"? Does it import the system module and work correctly with that? Or is it someone bypassing the Node.js module resolution? (In which case we may still want a resolve feature to disable/hook isCore().)

@ljharb
Copy link
Member

ljharb commented May 31, 2020

Indeed, this is correct. Only bundlers would use the installed one (which can be chosen via some function options you can read about in the readme); buffer/ would be how you'd do it in node and resolve by default.

@arikon
Copy link

arikon commented May 31, 2020

@octogonz typeorm depends on buffer package

@octogonz
Copy link

And I see they indeed do:

 window.Buffer = require("buffer/").Buffer;

@octogonz
Copy link

@ljharb In our case we are crawling dependencies and devDependencies from package.json. So if a name appears in package.json, we can reasonably assume that it is meant to be an NPM package, not a system module. And we could handle this by always appending / before calling resolve.sync().

This solution feels slightly clumsy, though. It would be ideal if resolve provided an explicit option to disable isCore(). Otherwise everywhere we do that, we have to include a code comment explaining the unfamiliar / suffix. And we have to make sure the string doesn't already contain /, etc.

@ljharb
Copy link
Member

ljharb commented May 31, 2020

You're right you'd need code like inPackageJSON(x) && isCore(x) ? `${x}/` : x, but that doesn't seem particularly troublesome?

@octogonz
Copy link

inPackageJSON(x) && isCore(x) ? `${x}/` : x isn't troublesome to write. Merely confusing for people who read the code later.

@octogonz
Copy link

octogonz commented Jun 3, 2020

Another problem is that the error messages come back like this:

ERROR: Cannot find module 'jquery/' from 'C:/my-project'

If I saw this error, my first thought would be "Oh, we must be accidentally appending a slash somewhere".

@ljharb
Copy link
Member

ljharb commented Jun 4, 2020

Why would the slash be appended for a non-core module? or do you mean, in the case where you unconditionally append a slash.

@ljharb
Copy link
Member

ljharb commented Dec 1, 2020

#233 has provided this.

@ljharb ljharb closed this as completed Dec 1, 2020
@octogonz
Copy link

octogonz commented Dec 2, 2020

Thanks for implementing this! We'll use it!

or do you mean, in the case where you unconditionally append a slash.

Yes, to skip core modules we had to unconditionally append a slash, and then the resulting error message would have a weird slash in it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants