Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

allow 'sys' to be an alias for 'util' #3577

Closed
bcantrill opened this Issue · 41 comments
@bcantrill
Owner

In the migration of Joyent's extensive node-based software stack to 0.8, we have found one of the most tedious (and enraging) issues to be the removal of 'sys' for 'util'. Yes, we know that this has been deprecated for quite some time -- but it doesn't change the fact that 'sys' has worked and that making this change was excruciatingly painful. (No, we didn't decide to assign 'sys' to the results of "require('util')" -- and if that were the recommendation, wouldn't that undermine the putative rationale for the removal of 'sys'?) To quantify the effect: we had to make thousands of changes across tens of repos -- changes that were exasperating, to say the least (with great opportunity for a slip-up that wouldn't manifest itself until, say, a critical error message resulted in an uncaught exception). History has taught us that the issues that we at Joyent experience are likely to be experienced by the broader node base; for this reason, we believe that 'sys' should be permitted as an alias for 'util' for the indefinite future.

@isaacs
Owner

I'm sort of +1 on this, maybe even in 0.8.

  1. It does not change ABI.
  2. "sys" is undocumented, and therefor an internal API.
  3. It won't break any programs to add module.exports = require('util'); as a sys module, and it will make some work.
  4. This is like 80% of the complaining I'm hearing about 0.8, and it's completely ridiculous. We should just declare that the throw is a bug, and call this a bugfix.

We put a few rounds in sys already. It'll probably die on its own. Is it really necessary to make it throw?

@isaacs isaacs closed this in f2a9ed4
@isaacs isaacs reopened this
@isaacs
Owner

(push was to reviewme branch, should not close this.)

@defunctzombie

I think it depends on how you feel about module deprecation in core. There are those out there that will say that once you ship code you must never break backwards compat, but I think this is a bit of a farce since things change over time and there are cleaner and better ways to do it.

I don't find the two cycle deprecation that unreasonable. Personally, if a module author can't be bothered to keep their module up to date, I think this is grounds for removal from npm. Modules shouldn't go to npm to be published once and die a slow death, they should thrive :)

@indutny
Owner

-1 for this.

Things should depricate, otherwise we'll end up with what we've now in browsers. If people decided to migrate to the new stable version of node, they should really do a migration, not just build/install new version.

@jfhbrook

Just deprecate it. People had months of warnings and we even wrote a bot to fix it for them. All they have to do is click a green button.

@luk-

-1. I'm also disappointed to see this back in for the reasons @shtylman and @indutny mentioned. I can see why it would be a frustrating change for Joyent but I think it's a good step for node and the community.

@bcantrill
Owner

An important part of becoming a more mature software platform is having empathy with those who use and deploy upon that platform. This is breaking users -- indeed, the early adopters -- over nothing. (I would call this a point of principle, but I honestly don't know what the principle is.) This is -- as Isaac mentioned -- 80% of the complaining about 0.8.0. You may say that "a bot will fix them", but that doesn't fly when your software is enterprise-grade production software. Again, this is stupid -- and (worst of all) it lends credence to some of the stereotypes about node (stereotypes that people like Isaac and I have fought very hard to dispel).

@defunctzombie

"enterprise-grade production software" what does that even mean?

I think the developers had enough empathy. A version of deprecation and then removal. I think the migration docs are pretty clear on the matter. Yes, we are all busy people.

@jfhbrook

fwiw this hasn't been much of an issue at nodejitsu outside a few stale cloudhead libraries that we try not to use anyway. Then again, we also test everything out on 0.8 so it won't be a disaster when we offer it in production.

@Sannis

@bcantrill you can solve this with sys-not-throw and only two lines for each executable instead of replace s/sys/util/ over all your code. But I think it should not be a degradation from deprecated state to module.exports = require('util'); without any warning.

@luk-

@bcantrill do you think this should ever be removed? How indefinite are you talking about in regard to indefinite future?

I empathize with your comment on working with enterprise-grade stuff, but is a < 1.0 release not a good time to deprecate? I have no idea how 1.0 and beyond releases will be managed and if it will be any different, but now seems like a great time to 'mess' with core modules before the possibility of versioning being more in control of the type of thing.

@isaacs
Owner

"enterprise-grade production software" what does that even mean?

It means "we have a lot of programs running systems that companies are paying a lot of money to use, and which are error-prone and tedious to upgrade". Like, many datacenters, thousands of servers, international partners, etc.

Let's be reasonable about this:

  1. What is the cost of keeping "sys" throwing?
  2. What is the cost of putting it back?

The cost of 1 is "Increased difficulty of migrating code from 0.6 (or earlier) to 0.8." This is harmful for the project long term. It also results in greater skepticism about migrating from 0.8 to 0.next or 1.0. It injures our credibility when we say, "No, it'll work, trust me."

The cost of 2 is "A user might ask someday, 'Why did you do require('sys') instead of require util?' and you'll have to tell them." They'll get it. That's not a real thing. However, it will perhaps injure our credibility when we say, "XYZ is deprecated. Don't use it. It'll be removed", because people will think, "Yeah, but that's what you said about sys, and that still works."

So, either way, we suffer a reduction in credibility with respect to a specific sort of message. Which message do you think we'll be saying more of in the future?

  1. "Please upgrade to this new version, it'll be better, I promise."
  2. "Please don't use this feature, it will be removed."

Unless someone can point to a real hazard of making it not throw, this doesn't even seem debatable, really. The arguments against this proposal are basically, "But that's not what we do". And maybe we should rethink what we do, if "what we do" is making it harder for people to use node.

@mmalecki

I'm very -1 on that. That way, we will end up reverting every compatibility breakage because "people were using it" (or, even better, "enterprise was using it"). sys was supposed to be totally removed in 0.9 (as in, it'd throw Cannot find module). That's the only sane thing we can do right now.

Learn to fix your code. Node is a fast moving project. APIs will break.

Also, as @jesusabdullah said, there's a bot which does that for you. Just put it on GitHub.

@sandfox

-1

While maybe it shouldn't throw yet... (personally I think it should but I'm aggressive), the "refusing to break backwards compatibility" culture is the sort of thing that made PHP the mess it is today (wasn't the only thing and it is being fixed - slowly).

In an ideal world APIs would only break on major version numbers but seeing as node has yet to reach 1.x.x status I think it's acceptable to break across minor version numbers. The culture after all is fast moving.

(Maybe deprecate in one minor version number and then nuke altogether in the next?)

Just my two pennies worth

@piscisaureus

I don't mind keeping it around for a little longer, or even for 2 years. But my question would be: when are we going to remove it if we don't remove it right now? I do think it should print a deprecation warning though.

It is my experience though that as long as stuff keeps working, nobody will change it. So if we really remove it (again) in version 0.12 or whatever then people will still be pissed :-)

@jfhbrook

I guess I kinda see this as the last thing we wanted to ditch. The migration from 0.6 to 0.8 was certainly much easier than from 0.4 to 0.6 even with sys throwing.

That said, I see why making it an alias is a reasonable thing to do, and while I don't really need it I wouldn't really have a conniption if it stuck around. So, I feel fairly ambivalent about this decision.

@defunctzombie

If you are not gonna change api in 0.x releases then don't change API. having some sort of selective enforcement about we can change this.. but we will back out on this seems worse than no policy.

What if I didn't like one of the other changes in 0.8? How much money do I have to throw at the "enterprise" before my request is taken seriously?

@bcantrill
Owner

When you innovate in systems software, you will make changes that break people. The challenge is to restrict those changes to those which are absolutely necessary -- where supporting the old way of doing things is truly constraining for future development. This is not an example of that; supporting 'sys' as an alias for 'util' has no real cost going forward. And to Isaac's point: breaking it does have a cost. That cost is both immediate (changes need to be made), but the deeper cost is the erosion of trust: if node breaks apps for capricious reasons, you will find a reluctance to upgrade and adopt -- and ultimately you will find significant portions of the community disaffected and yearning for an alternative. Part of maturing as a platform is accepting that this trust is important, and violating only when you absolutely must.

@isaacs
Owner

Discussed with @piscisaureus. "sys" is going to be un-deprecated. There's really no reason to remove the alias.

The message of @shtylman and others in this thread is not un-heard, though. We need to clarify our policies. I don't want node to turn into PHP, either, and we're certainly not going to un-make breaking changes every time it upsets someone.

@isaacs
Owner

(closed by f2a9ed4)

@mikeal

if switching from sys to util now is painful then the pain will only grow with time.

we should be plain and honest about this decision, it's forever. this will never change, and be something of a wart forever, and we should be ok with that.

@x3ro

-1 @sandfox already said what I would've said :)

@Raynos

-1 API surface bloat like this has little value.

If "enterprise applications" really have too much pain updating their code then these kind of backwards compatibility hacks should be behind some kind of optional compile flag and not there by default.

Alternatively support an optional compilation flag for a leaner API with all the legacy cruft gone. Something like ES5 strict mode where you can basically opt in to remove legacy cruft.

@isaacs
Owner

we should be plain and honest about this decision, it's forever. this will never change, and be something of a wart forever, and we should be ok with that.

Yes, that's why I also removed the warning :)

If there ever comes a time when having ONE EXTRA LINE of javascript in node is worth throwing errors in obscure places all over the place... well, we'll probably have bigger problems. But we'll just deal with that then. I am betting that wont' ever happen.

@dominictarr

I am really pleased how many people are against this.
Although the arguments for changing this back are strong,
it shows that the node community accepts a culture of change.

@isaacs
Owner

@dominictarr Yeah, that is kind of badass :) Unfortunately, sometimes the real world means a bit of compromise.

@chapel

I have talked on IRC a bit about this with @isaacs @dominictarr @jesusabdullah and others, the main take away being that there is no downside to making it not throw. The only perceived downside is that some sort of principle was lost, but even that is meaningless.

sys should have been removed long ago, no warnings, just gone. If you let something sit there and still work, no one will change it. Why? Because it doesn't break. Now add in a year or more of not breaking, and you have people with legacy code that otherwise would work with the new version except for sys is everywhere. It is their fault for continuing to use it, but it is also our fault for letting it sit and not having it removed earlier.

By the way I support yanking it out, but don't think it is worth a huge fight. Let's bike shed about something else. Maybe es6 modules?

@polotek

+1

@medikoo

If it's so hard on some enterprise platforms to upgrade outdated software, why it's so easy to upgrade Node.js there? Shouldn't it come together?

I'm also providing software to some enterprise ends, and I don't see the issue, in documentation it's clearly stated that application needs Node.js v0.6. If next Node pop ups and I have some worthwile updates I inform the client that we can proceed and update both (Node.js and application). If it's problematic, then ok, we don't touch anything.

@isaacs
Owner

@medikoo That's a great question!

The problem is that they're both hard to upgrade. But look at it from a business perspective, it makes perfect sense:

  1. You built the legacy stuff in-house, and whether you're a strapped startup, or a big company that had a whole team working on it, it's a non-zero cost to update it.
  2. Someone else already did most of the work upgrading node itself. (They haven't tested it on your system, so you'll probably hit some bumps, but still, it's pretty much done.)
  3. Node 0.8 has some advantages. Faster, stabler. You hope to reduce ongoing operational costs by servicing more requests, with fewer restarts, and that easily justifies a modest up-front cost.

So, we say to that user, "In order to upgrade you have to update all that legacy code." That isn't great, raises the price, but maybe justifiable. Surely, these changes were necessary requirements of the various improvements, right?

In most cases, you can say, "Why yes." In a few cases in 0.8, even when a breaking change WAS really necessary, we still went out of our way to preserve compatibility, because we gain by our users succeeding with our software. The project needs them to upgrade, or we lose them.

But in this case, is it strictly necessary? No. It's just that we all agreed 2 years ago that the green bikesheds are Wrong Bikesheds, cannot be repainted, and must instead be detonated. At this point, doing anything else would set a bad precedent.

@sandfox

Looking beyond this single issue of what to do about 'sys', which can become a one-off historical oddity if needs be, what is the policy going forward on breaking stuff going to be? I think some consistency here is the sort of thing 'enterprise' kids would value. They dislike unpredictable more than they dislike big change.

Also - should this become another issue or something as its possibly a little off topic here.

@bcantrill
Owner

Isaac addressed this in his comprehensive mail on the subject:

https://groups.google.com/group/nodejs/msg/8a0ba61e2716275e?hl=en%3Fhl%3Den

@sandfox

@bcantrill thanks, for some reason hadn't seen it - work's spam-filter got hungry :-)
Also nice to have it linked to here for future reference.

@oddmarthon-lende oddmarthon-lende referenced this issue from a commit in oddmarthon-lende/node
@isaacs isaacs 2012.06.29, Version 0.8.1 (stable)
* V8: upgrade to v3.11.10.12

* npm: upgrade to v1.1.33
  - Support for parallel use of the cache folder
  - Retry on registry timeouts or network failures (Trent Mick)
  - Reduce 'engines' failures to a warning
  - Use new zsh completion if aviailable (Jeremy Cantrell)

* Fix #3577 Un-break require('sys')

* util: speed up formatting of large arrays/objects (Ben Noordhuis)

* windows: make fs.realpath(Sync) work with UNC paths (Bert Belder)

* build: fix --shared-v8 option (Ben Noordhuis)

* doc: `detached` is a boolean (Andreas Madsen)

* build: use proper python interpreter (Ben Noordhuis)

* build: expand ~ in `./configure --prefix=~/a/b/c` (Ben Noordhuis)

* build: handle CC env var with spaces (Gabriel de Perthuis)

* build: fix V8 build when compiling with gcc 4.5 (Ben Noordhuis)

* build: fix --shared-v8 option (Ben Noordhuis)

* windows msi: Fix icon issue which caused huge file size (Bert Belder)

* unix: assume that dlopen() may clobber dlerror() (Ben Noordhuis)

* sunos: fix memory corruption bugs (Ben Noordhuis)

* windows: better (f)utimes and (f)stat (Bert Belder)
2134aa3
@jcayzac

-1 new versions shouldn't care about old users, for those can still use old versions if they really don't want to update their code base. Disappointed it made it in 0.8.1.

@AndreasMadsen

1+ on this patch, there is no harm done except some proud and that I think is the real problem here.

I can't believe that Joyent as an Node.js supporter and trademark owner, didn't test there software before 0.8.0 landed. They of all companies should have tested there platforms and created this issue before 0.8.0 landed. I also feel that isaacs gives a really bad statement when it writes:

We should just declare that the throw is a bug, and call this a bugfix.

Now, I know for a fact that this has been discussed many times, and to suddenly call it a bug is not justifiable. Call it _ a good decision there was bad in production and must be reverted_ but not a bug. Because the core team was very much aware that this should throw when the decision was made.

This is only how I fell, there is no right and wrong and there can be no justice as long as law is absolute! However I will personally think twice before speaking good about Joyent, that is the only true sadness today.

@isaacs if often find your perspective more inspiring and enlightning than any other programmer I have very heard and do now and then ask my self "how would isaacs analyse this", it is good to see that you too are a human being. I hope you will once again use the mistaces (whatevery it might be in this case) and reflect on them. That reflection is the only response I hope to hear from you in this matter.

@isaacs
Owner

I can't believe that Joyent as an Node.js supporter and trademark owner, didn't test there software before 0.8.0 landed.

Joyent's been using 0.7 for a few weeks now, and did test 0.8.0 before it went live. In fact, Joyent's code has all been switched over to require('utils') as far as I know. Seeing the unnecessary pain of doing this, for no real benefit, helped the decision to go back on it. (I know @joshwilsdon is probably pissed that he did all that work for nothing.)

Predicting having to deal with this in all our customers' code, etc., just really made it clear that this is not worth it.

Now, I know for a fact that this has been discussed many times, and to suddenly call it a bug is not justifiable.

Yes, that was a joke, don't worry :) We're not going to start playing legal games with our policies. 'util' is arguably a better name than 'sys'. The real mistake was that we needed to just delete it back in 0.2 if that was what we were gonna do.

@jfhbrook

You know what's really cool? We don't have to explain this to anybody anymore:

Q:

I upgraded from node v0.6.12 to 0.80 and started receiving the error below - I have removed the sys module from the
import but am still getting the error. Any helpful hints would be helpful. I am not suing stylus either.

A:

Upgrade to node v0.8.1. It will solve your problem

Q:

I upgraded to 0.8.1 and it resolved the issue.

I definitely prefer this over sticking to ideology here. XD

http://stackoverflow.com/questions/11276848/upgraded-node-to-v0-8-and-started-receiving-an-error-for-sys-util

@jcayzac

(I know @joshwilsdon is probably pissed that he did all that work for nothing.

Changing require('sys') to require('util') in all source files across all projects on a bazillion mountpoints is just one line of bash code, so this made me laugh.

@polotek
@AndreasMadsen

Joyent's been using 0.7 for a few weeks now, and did test 0.8.0 before it went live. In fact, Joyent's code has all been switched over to require('utils') as far as I know.

It still dosen't justify creating this issue after 0.8.0 landed (I'm glad they did, better late than never) but it is bad style. If the goal was an easy migration, the issue should have been apparent immediately and in that moment an issue should have been created.

@tralamazza tralamazza referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@piscisaureus

Fixed in f2a9ed4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.