FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

module: remove unnecessary property and method by thefourtheye · Pull Request #2922 · nodejs/node · GitHub

/ node Public

module: remove unnecessary property and method - #2922

Closed
thefourtheye wants to merge 1 commit into
nodejs:masterfrom
thefourtheye:clean-up-module
ConversationCommits (1)ChecksFiles changed
Closed

module: remove unnecessary property and method#2922
thefourtheye wants to merge 1 commit into
nodejs:masterfrom
thefourtheye:clean-up-module

Conversation

thefourtheye commented Sep 16, 2015

Copy link
Copy Markdown
Contributor

require.paths property and require.registerExtension function have
been throwing errors when used. They both are like this for years now.
This patch removes them from the system.

require.paths: 7f0047c#diff-d1234a869b3d648ebfcdce5a76747d71R359

require.registerExtension was removed even before that.

module: remove unnecessary property and method
7101620 
`require.path` property and `require.registerExtension` function have
been throwing errors when used. They both are like this for years now.
This patch removes them from the system.
thefourtheye added the module Issues and PRs related to the module subsystem. label Sep 16, 2015
mscdex added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 17, 2015

thefourtheye commented Sep 18, 2015

Copy link
Copy Markdown
Contributor Author

CI Run: https://ci.nodejs.org/job/node-test-pull-request/341/

thefourtheye commented Sep 22, 2015

Copy link
Copy Markdown
Contributor Author

Bump!

silverwind commented Sep 23, 2015

Copy link
Copy Markdown
Contributor

LGTM

thefourtheye commented Sep 28, 2015

Copy link
Copy Markdown
Contributor Author

I'll land this tomorrow if there are no objections. cc @nodejs/collaborators

indutny commented Sep 28, 2015

Copy link
Copy Markdown
Member

cc @rvagg what does our deprecation policy says on this?

mscdex commented Sep 28, 2015

Copy link
Copy Markdown
Contributor

LGTM

seishun commented Sep 28, 2015

Copy link
Copy Markdown
Contributor

Why semver-major though? It was already removed. It's not like any application will break because of this change.

thefourtheye commented Sep 28, 2015

Copy link
Copy Markdown
Contributor Author

@seishun True, if anyone is using it then their application will just fail. @mscdex What do you think?

ChALkeR commented Sep 28, 2015

Copy link
Copy Markdown
Member

@thefourtheye It was require.paths, not require.path.

Trott reviewed Sep 28, 2015
View reviewed changes
Comment thread test/sequential/test-module-loading.js

Trott Sep 28, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit: Can you add a test to confirm that require.paths returns undefined or whatever the expected value is?

Trott Sep 28, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit: Probably good to do the same for require.registerExtension().

thefourtheye Sep 28, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@Trott Do we really need that? There will be no reference to that in code, but there will be a test which will check if paths is undefined.

Trott Sep 28, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

My thinking is this PR changes the behavior of the code and there ought to be a test for that change. So a test that the property has, in fact, been successfully removed seems appropriate. But if you think it's overkill, I'm OK without it. (I'll edit my comments above to indicate they are nits.)

thefourtheye Sep 28, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@Trott Hmmm, whenever we remove something from the system we don't add tests to make sure that they are really removed, right? So, I feel that it is not necessary. Let's see what others feel.

thefourtheye commented Sep 28, 2015

Copy link
Copy Markdown
Contributor Author

@ChALkeR Ah, thanks for pointing out. I edited the PR description. I'll update the commit log when I am landing this.

ChALkeR commented Sep 28, 2015

Copy link
Copy Markdown
Member

@seishun It changes the behavior by removing a throw. If anyone did a feature detection checking whenether require.paths throws or not, it would be broken by this commit. While the likehood of such situation is near zero, it still makes this PR a semver-major IMO.

ChALkeR commented Sep 28, 2015

Copy link
Copy Markdown
Member

require.registerExtension( usage:

amdify-0.0.26.tgz/jam/ejs/lib/ejs.js:350:  require.registerExtension('.ejs', function(src) {
backbone-revisited-0.1.2.tgz/node_mods/ejs.js:353:  require.registerExtension('.ejs', function(src) {
blitzLib-0.1.0.tgz/Lib/coffee-script-debug.js:14330:    }: require.registerExtension && require.registerExtension(".coffee",
bluebutton-0.4.2.tgz/_site/spec/javascripts/helpers/ejs.js:411:  require.registerExtension('.ejs', function(src) {
clientexpress-0.7.7.tgz/lib/ejs.js:301:  require.registerExtension('.ejs', function(src) {
coffeete-0.0.4.tgz/src/coffeete.js:22:        require.registerExtension('.coffeete', function (content) {
contracts.coffee-0.3.4.tgz/lib/coffee-script/coffee-script.js:24:    require.registerExtension('.coffee', function(content) {
ejs-harmony-1.0.1.tgz/lib/ejs.js:413:  require.registerExtension('.ejs', function(src) {
ejs-i-1.2.3.tgz/ejs.js:417:      require.registerExtension('.ejs', function (src) {
ejs-i-1.2.3.tgz/lib/ejs.js:399:  require.registerExtension('.ejs', function (src) {
ejs-remix-0.1.1.tgz/ejs.js:346:  require.registerExtension('.ejs', function(src) {
ejs-remix-0.1.1.tgz/lib/ejs.js:504:  require.registerExtension('.ejs', function(src) {
ejs-template-0.1.0.tgz/ejs/ejs.js:376:  require.registerExtension('.ejs', function(src) {
ejs-var-1.0.0.tgz/ejs.js:411:  require.registerExtension('.ejs', function(src) {
ejs-var-1.0.0.tgz/lib/ejs.js:374:  require.registerExtension('.ejs', function(src) {
ender-ejs-0.6.1.tgz/ejs.js:334:  require.registerExtension('.ejs', function(src) {
funtang.compiler-0.0.1.tgz/reference/jashkenas-coffee-script-60c9b94/lib/coffee-script/coffee-script.js:25:    require.registerExtension('.coffee', function(content) {
gorillascript-0.9.10.tgz/extras/gorillascript.js:30580:          require.registerExtension(".gs", function (content) {
gorillascript-0.9.10.tgz/lib/gorilla.js:435:    require.registerExtension(".gs", function (content) {
imba-0.13.0.tgz/register.js:39: require.registerExtension('.imba',function (content){
logmonger-0.1.0.tgz/contrib/coffee-script/coffee-script.js:14:    require.registerExtension('.coffee', function(content) {
mochiscript-0.6.16.tgz/lib/mochiscript/mochiscript.js:1280:  require.registerExtension('.ms', function(content, filename) {
neode-0.0.0.tgz/coffee-script/lib/coffee-script/coffee-script.js:25:    require.registerExtension('.coffee', function(content) {
ngine-0.0.2.tgz/lib/ejs.js:387:  require.registerExtension('.ejs', function(src) {
periodic.component.list-view-scroll-0.1.1.tgz/example/scripts/example.js:2827:  require.registerExtension('.ejs', function(src) {
sdejs-0.0.2.tgz/lib/sdejs.js:376:  require.registerExtension('.ejs', function(src) {
sdejs-0.0.2.tgz/sdejs.js:420:  require.registerExtension('.ejs', function(src) {
springbokejs-0.0.1.tgz/ejs.js:346:  require.registerExtension('.ejs', function(src) {
springbokejs-0.0.1.tgz/lib/ejs.js:378:  require.registerExtension('.ejs', function(src) {
tinyliquid-0.2.29.tgz/other/ejs.js:411:  require.registerExtension('.ejs', function(src) {
toffee-0.1.12.tgz/lib/coffee-script/coffee-script.js:25:    require.registerExtension('.coffee', function(content) {
TwigJS-0.0.2.tgz/support/coffee-script/lib/coffee-script/coffee-script.js:17:    require.registerExtension('.coffee', function(content) {
twigjs-0.0.4.tgz/support/coffee-script/lib/coffee-script/coffee-script.js:17:    require.registerExtension('.coffee', function(content) {
utml-0.2.0.tgz/utml.js:224:  require.registerExtension('.utml', function(src){
vork-0.0.12.tgz/exampleApp/app/requirejsBuilder/plugins/ejs.js:333:                require.registerExtension('.ejs', function(src) {
wsi-ejs-1.2.0.tgz/ejs.js:413:  require.registerExtension('.ejs', function(src) {
wsi-ejs-1.2.0.tgz/lib/ejs.js:361:  require.registerExtension('.ejs', function(src) {
yerbascript-0.0.1.tgz/lib/coffee-script/coffee-script.js:31:    require.registerExtension('.yerba', function(content) {
yerbascript-0.0.1.tgz/lib/coffee-script/coffee-script.js:34:    require.registerExtension('.coffee', function(content) {

Most of them do an } else if (require.registerExtension) { check before calling require.registerExtension.

mscdex commented Sep 28, 2015

Copy link
Copy Markdown
Contributor

IMHO I would just remove them both, they have been deprecated for a long time now, at least since 2011. It's not like it was a soft deprecation either.

ChALkeR commented Sep 28, 2015

Copy link
Copy Markdown
Member

require.paths usage: https://gist.github.com/ChALkeR/4d1f25b54e5786dcca81

thefourtheye commented Sep 28, 2015

Copy link
Copy Markdown
Contributor Author

@ChALkeR Are all those packages latest?

ChALkeR commented Sep 28, 2015

Copy link
Copy Markdown
Member

@thefourtheye For 2015-09-21, yes.

thefourtheye commented Sep 28, 2015

Copy link
Copy Markdown
Contributor Author

hmmm, I wonder how they are really working.

ChALkeR commented Sep 28, 2015

Copy link
Copy Markdown
Member

There could be a lot of false positives for require.paths — the code could be performing feature detection to be compatible with old versions, «require» and «paths» are common words, so it might be a different «require», some lines are actually comments or strings.

Also some modules are not used by anybody.

ChALkeR commented Sep 28, 2015

Copy link
Copy Markdown
Member

@thefourtheye Also note that those are only 1100 lines in 442 modules for require.paths. From 186231 modules on npm. That's actually 0.2%. Together with the false positives.

rvagg commented Sep 29, 2015

Copy link
Copy Markdown
Member

cc @rvagg what does our deprecation policy says on this?

Not sure we have coverage of this kind of situation because it's not even deprecated, it's flat-out broken. I could imagine a situation where you're using the exception in some upstream workflow but you'd have to be more than a little crazy to be doing that I think..

I bet lot of those packages are either not attempting to use that functionality (notice how many have ejs.js or coffee-script.js in a lib directory?) or are simply abandonware.

I'm +0.5 on this as long as it's only for v5+.

thefourtheye commented Oct 2, 2015

Copy link
Copy Markdown
Contributor Author

@ChALkeR LGTY?

ChALkeR commented Oct 4, 2015

Copy link
Copy Markdown
Member

@thefourtheye
LGTM for a major.

Also, most of those require.paths usage lines are throwing an error without this PR (in 4.x) and will throw an error (though a different one) with this PR (in 5.x), as they attempt to do require.paths.something(…), for example require.paths.unshift(…).

Fishrock123 commented Oct 5, 2015

Copy link
Copy Markdown
Contributor

LGTM for v5

Fishrock123 added this to the 5.0.0 milestone Oct 5, 2015

bnoordhuis commented Oct 5, 2015

Copy link
Copy Markdown
Member

LGTM

thefourtheye added a commit that referenced this pull request Oct 6, 2015
module: remove unnecessary property and method
6ad458b 
`require.paths` property and `require.registerExtension` function have
been throwing errors when used. They both are like this for years now.
This patch removes them from the system.

PR-URL: #2922
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
RickAllMighty8195 mentioned this pull request Feb 11, 2024
Open
Megoy-Tambayan mentioned this pull request Feb 11, 2024
Open
Dixiejane mentioned this pull request Feb 11, 2024
Open
JCsomeShots mentioned this pull request Feb 11, 2024
Open
jj-crypto mentioned this pull request Feb 11, 2024
Open
ZRDK mentioned this pull request Feb 11, 2024
Open
buzzlightyear726 mentioned this pull request Feb 11, 2024
Open
Renukaloch mentioned this pull request Feb 11, 2024
Open
MaxMood96 mentioned this pull request Feb 11, 2024
Open
davidmather mentioned this pull request Feb 11, 2024
Open
mrod0101 mentioned this pull request Feb 11, 2024
Closed
CxsLucyfer mentioned this pull request Feb 11, 2024
Open
dewaynesmith3711 mentioned this pull request Feb 11, 2024
Open
kingjay66 mentioned this pull request Feb 11, 2024
Open
zhangpan0907 mentioned this pull request Feb 11, 2024
Open
Supermajestcd mentioned this pull request Feb 12, 2024
Open
MatheusKael mentioned this pull request Feb 12, 2024
Open
SanchitMahajan236 mentioned this pull request Feb 12, 2024
Open
terrorizer1980 mentioned this pull request Feb 12, 2024
Open
myvali998 mentioned this pull request Feb 12, 2024
Open
ambai mentioned this pull request Feb 12, 2024
Open
dewaynesmith3711 mentioned this pull request Feb 12, 2024
Open
BryanLaura mentioned this pull request Feb 12, 2024
Open
nicholaschoi168 mentioned this pull request Feb 12, 2024
Open
jefeaguila mentioned this pull request Feb 12, 2024
Open
Michaela813 mentioned this pull request Feb 12, 2024
Open
chrismilleruk mentioned this pull request Feb 12, 2024
Open
Michaela813 mentioned this pull request Feb 12, 2024
Open
JoanKalanzi1 mentioned this pull request Feb 12, 2024
Open
BryanLaura mentioned this pull request Feb 12, 2024
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

module Issues and PRs related to the module subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

10 participants

Back | FazBrowse Home | New Git URL