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

http, tls, stream, test, build: refactor lib/ underscore files into directories #16745

Closed
wants to merge 1 commit into from

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Nov 4, 2017

this is really just a small cleanup, no actual functionality was
changed. I think this would mostly be a nice change for people who are
new to the node.js source, as peeking into /lib with all that can be a
bit confusing unless you understand how modules's builtin resolving works.

This will also require a change to nodejs/github-bot

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

http, tls, stream

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 4, 2017
@mscdex mscdex added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 4, 2017
@mscdex
Copy link
Contributor

mscdex commented Nov 4, 2017

unless you understand how NativeModule's builtin resolving works

I don't understand this part. It works the same as loading a module from disk. require('tls') loads lib/tls.js, require('internal/freelist') loads lib/internal/freelist.js, etc. There is nothing magical when it comes to resolving built-in modules.

@apapirovski
Copy link
Member

apapirovski commented Nov 4, 2017

Sorry but I'm -1 on this — seems like churn for pretty much no benefit. I would understand if we literally had 10s or 100s of files here but that's not even close to being the case. This also breaks anyone using --expose-internals (even if I have little sympathy for it). Changes like this can make backporting harder than it needs to be as well.

Given we haven't been able to come to an agreement on creating sub folders for tests, I don't see how the core code is going to be easier.

@devsnek
Copy link
Member Author

devsnek commented Nov 4, 2017

@mscdex when i first encountered /lib i was confused about why require('_http_common.js') didn't work until i had snooped out the entire module resolving pipeline and found the static list of builtin modules

@mscdex
Copy link
Contributor

mscdex commented Nov 4, 2017

@devsnek If anything, that sounds like more of a documentation change PR for the contributing guide or something. This PR does not change how built-in modules are loaded, so the same "issue" still exists...

@devsnek
Copy link
Member Author

devsnek commented Nov 4, 2017

@mscdex thats a good point, maybe i'll make a pr to that as well, but this is also just a nice change. I agree it isn't needed, but it keeps the source organized, which is always a nice thing.

@mscdex
Copy link
Contributor

mscdex commented Nov 4, 2017

@devsnek Maybe, but IMHO it's not worth the potential breakage.

@jasnell
Copy link
Member

jasnell commented Nov 5, 2017

At this point, I'm -1 on this due to the extremely likely chance this will break existing userland code... which is unfortunate. What I'd like to see us be able to do is move these _-prefixed modules into internal at some point instead, then do a runtime deprecation of the user-requirable versions.

Even if this PR does move forward, a deprecation for the current require paths would be a requirement to land.

@mcollina
Copy link
Member

mcollina commented Nov 5, 2017

There is #11957 that is prior art on this.

@devsnek
Copy link
Member Author

devsnek commented Nov 5, 2017

I can definitely add stubs, and maybe moving them to internal is a good idea to give more freedom in the future

@BridgeAR
Copy link
Member

It seems like there are multiple -1 and it is not likely that this is going to land. I am closing this therefore.

@BridgeAR BridgeAR closed this Dec 10, 2017
@devsnek devsnek deleted the refactor/lib-parts branch January 29, 2018 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants