module: deprecate require('module').Module

[JacksonTian]

recommend developers to use require('module').

[mscdex]

That's not true, require('module').Module definitely is accessible outside of core. However, we would need to run some checks against npm modules to see if they use that or not before removing it. At the very least we'd need to add a deprecation warning. Keeping it around doesn't add any burden though.

[jasnell]

+1 to @mscdex's comments. We cannot simply remove it like this. There is a required deprecated cycle we'd need to go through and even then, once marked deprecated it could end up sticking around for quite a while (even if it's a bit silly having it there)

[JacksonTian]

Thanks, I will update it.

[jasnell]

LGTM

[mscdex]

After this week's CTC meeting discussion about the meaning of "deprecation", should this PR be changed to more accurately reflect the status/intention? Or are we going to commit it now and change the messaging later? Or something else?

/cc @nodejs/ctc

[cjihrig]

You could use a template string to avoid escaping the quotes.

[cjihrig]

@mscdex did you have something specific in mind?

[jasnell]

Given that the deprecation message doesn't say anything about it being removed and is consistent with the other deprecation messages we have, I'd say it's likely safe to land as is now and work out the revised messaging around deprecation later

[mscdex]

@cjihrig I mean if our intention is to remove it, we should be using "stronger" wording?

[cjihrig]

I think this is fine for now. We could do one sweeping PR later to update all the deprecation messages once we know what we want to say (maybe add "and will be removed" to the message).

[trevnorris]

Just curious, why this and not include others like require('events').EventEmitter?

[JacksonTian]

@trevnorris If this patch can be accepted, I will do that.

[cjihrig]

LGTM for v6.

[evanlucas]

looks like npm init will be affected by this (https://github.com/npm/promzard/blob/master/promzard.js#L12).

There are more than just that, waiting on grep to finish...

EDIT: Here is the first batch https://gist.github.com/evanlucas/07e4771bbf7a83ca3b16

These are a little bit old, btw

[ChALkeR]

Quick and dirty grep results: https://gist.github.com/ChALkeR/b13b22e813157b4b49d6

[ChALkeR]

@evanlucas You can use my dataset for finding those, it's uploaded long ago to http://oserv.org/npm/Gzemnid/, takes up less than 3 GiB, and an average grep time is about 60-70 seconds.

It ignores dependencies though, and searches for the match only in the topmost module sources (node_modules is excluded).

[ChALkeR]

Modules API has Stability: 3 - Locked per doc.
While this PR is unlikely to break anything or affect many modules (the usage seems low), the documentation states that this PR should be rejected.

If Modules API was not Locked, I would vote +1 for this PR.

[evanlucas]

Along with the module system being locked, this affects the package manager that is bundled with node. -1 from me as it doesn't really gain us anything...

[jasnell]

Any updates on this one?

[jasnell]

@nodejs/ctc