url: remove unused URL::ToObject() method

[bnoordhuis]

Alternative to #14096.

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

[bmeck]

Used in ESM

[jasnell]

-1... this was added to support ongoing future development work. It should not be removed.

[bnoordhuis]

I've commented on that in #14096. Suffice to say I think it's plain stupid¹ to land unused code. To add more insult, unused code with glaring bugs in it. The "but it's going to be used any day now" argument doesn't hold water, this code has been in master for months.

I'm keeping this PR around in case that ESM PR doesn't materialize in the next few days. Consider it a Sword of Damocles.

¹ In case you take exception to that word, take comfort in the fact that I thought for quite some time about alternative wording and couldn't come up with anything that still does justice to the way I feel about this.

[bmeck]

@bnoordhuis is that some sort threat to land this regardless of how others feel?

[bnoordhuis]

Not a threat, I'm just not closing it just yet. I'll put it before the CTC if this ESM PR doesn't happen quick enough unless @jasnell changes his mind in the mean time.

[gibfahn]

Agreed that it shouldn't have landed without the code that uses it (unless there's a particular reason to). However now it's here removing it adds churn, so unless ESM is >6months away from master it doesn't seem worth removing.

[bnoordhuis]

@bmeck Status update? 'Next day or two' is almost two weeks ago now.

[bmeck]

Death in family, dont care. Keep pestering.

…

On Jul 17, 2017 5:37 AM, "Ben Noordhuis" ***@***.***> wrote: @bmeck <https://github.com/bmeck> Status update? 'Next day or two' is almost two weeks ago now. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14106 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOUo2CCdGsGIdg-WTkvPxQwoUe6klyHks5sOzl0gaJpZM4OPt2Y> .

[jasnell]

I'm still -1 on removing this. There's no reason to do so, the small amount of code is harmless, adds zero burden on anyone, and is planned for use. Anyone who feels strongly enough can ask for a vote, of course.

[bnoordhuis]

@bmeck Sorry to hear that, take care.

@jasnell I remember being baffled about the existence of that method before while reviewing another PR. Since 'zero burden' is not in fact the case and 'planned for use' may not be any time soon, will you reconsider?

[targos]

I see no problem in removing it. This commit can just be reverted when the method is needed.

[bnoordhuis]

@bmeck @jasnell Is URL::ToObject() going to be used in #14369 or do I need to put this to a vote at the next CTC meeting?

[bmeck]

@bnoordhuis https://github.com/nodejs/node/pull/14369/files#diff-4a5950be44f56dfca2e7fa3c4e1fc0e4R497

[jasnell]

@bnoordhuis ... Yes.

[jasnell]

-1, API is being used.

[TimothyGu]

This is getting silly. The function works, and there is an open PR using this function. It would be counterproductive to remove it now. Closing.

[bnoordhuis]

@TimothyGu Well, duh - that's why I asked the question. But read up on the history, it's been defective for most of its lifetime and rather glaringly so. I don't want this to happen again.