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

disallow names of node core modules #2

Merged
merged 4 commits into from
Jan 20, 2015
Merged

disallow names of node core modules #2

merged 4 commits into from
Jan 20, 2015

Conversation

zeke
Copy link
Contributor

@zeke zeke commented Nov 18, 2014

For the consideration of @othiym23 and @iarna, I present a possible implementation of #1.

Twas a good excuse to write node-core-module-names, my smallest npm package yet.

cc @robertkowalski

@iarna
Copy link

iarna commented Nov 18, 2014

Ok but currently this is a warning and AFAIK it needs to stay that way as we still have some packages published under core module names (see buffer)

@zeke
Copy link
Contributor Author

zeke commented Nov 18, 2014

I see. @iarna, do you think the warning-ness should be the responsibility of this package? If so, maybe the response object could look like this:

{
  valid: false, 
  errors: ['foo'],
  warnings: ['bar']
}

@maxogden just pointed out to me that there's already a thing that is the same as node-core-package-names: https://preview.npmjs.com/package/builtins

@feross
Copy link

feross commented Nov 21, 2014

Yeah, disallowing core module names is a bad idea, but warning is fine (that's already the current npm behavior).

Browserify, and many others, depend on modules with names that match core. Examples:

To depend on modules that match core names, you just append a forward slash. Like this: var Buffer = require('buffer/').Buffer This feels a bit janky, but the require code is locked (stability index 5) and will probably never change.

@iarna
Copy link

iarna commented Dec 15, 2014

@zeke Yeah, i'd be happy getting back an error and a warning list– that interface looks great.

@zeke
Copy link
Contributor Author

zeke commented Dec 18, 2014

I added support for warnings in the commits above. Take a look and let me know what you think.

@feross
Copy link

feross commented Dec 18, 2014

LGTM! 👍

zeke added a commit that referenced this pull request Jan 20, 2015
disallow names of node core modules
@zeke zeke merged commit 0b84cb3 into master Jan 20, 2015
@zeke zeke deleted the no-core-module-names branch January 20, 2015 00:39
@zeke
Copy link
Contributor Author

zeke commented Jan 20, 2015

Version 2 just landed with warnings and a new boolean for validity of legacy packages names. See https://www.npmjs.com/package/validate-npm-package-name#legacy-names

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

Successfully merging this pull request may close these issues.

None yet

3 participants