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

use os.tmpdir() polyfill for more consistent behaviour #50

Closed
wants to merge 1 commit into from
Closed

use os.tmpdir() polyfill for more consistent behaviour #50

wants to merge 1 commit into from

Conversation

sindresorhus
Copy link

The os.tmpdir() method has changed a lot between Node versions and it would be nice for users if it were consistent between Node versions. This might even prevent hard to track down bugs. It also has the added benefit of removing some code, which is always good.

Node 0.10.38: https://github.com/joyent/node/blob/0b5731a63cc40c4fe9275c79158fe0a5dd4d1609/lib/os.js#L44-L49

io.js 2.0.1: https://github.com/iojs/io.js/blob/6c80e38b014b7be570ffafa91032a6d67d7dd4ae/lib/os.js#L25-L40

From io.js changelog:

os.tmpdir() is now cross-platform consistent and will no longer returns a path with a trailling slash on any platform

Updated os.tmpdir on Windows to use the %SystemRoot% or %WINDIR% environment variables instead of the hard-coded value of c:\windows when determining the temporary directory location.


https://github.com/sindresorhus/os-tmpdir

@raszi
Copy link
Owner

raszi commented May 8, 2015

It is not a bad idea although as you probably know because you're the maintainer of that project that with this move we'll drop the 0.6 and 0.8 compatibility without adding much back.

The `os.tmpdir()` method has changed a lot between Node versions and it would be nice for users if it were consistent between Node versions. This might even prevent hard to track down bugs. It also has the benefit of removing some code, which is always good.

Node 0.10.38: https://github.com/joyent/node/blob/0b5731a63cc40c4fe9275c79158fe0a5dd4d1609/lib/os.js#L44-L49

io.js 2.0.1: https://github.com/iojs/io.js/blob/6c80e38b014b7be570ffafa91032a6d67d7dd4ae/lib/os.js#L25-L40

From io.js changelog:

> os.tmpdir() is now cross-platform consistent and will no longer returns a path with a trailling slash on any platform

> Updated os.tmpdir on Windows to use the %SystemRoot% or %WINDIR% environment variables instead of the hard-coded value of c:\windows when determining the temporary directory location.

---

https://github.com/sindresorhus/os-tmpdir
@sindresorhus
Copy link
Author

No, it's only failing because they have an outdated npm version which doesn't handle the caret ^ version specifier. I've fixed this now, and all tests are passing. It works fine on Node 0.6 and 0.8.

@raszi
Copy link
Owner

raszi commented May 10, 2015

Maybe it is the case but in your package.json it is explicitly says >= 0.10.0, see here.

@raszi
Copy link
Owner

raszi commented May 10, 2015

I would rather not introduce a new dependency because we could get rid of only 10 lines of really simple code.

@raszi raszi closed this May 10, 2015
@sindresorhus
Copy link
Author

Maybe it is the case but in your package.json it is explicitly says >= 0.10.0, see here.

That property is not enforced. It's just a recommendation.

I would rather not introduce a new dependency because we could get rid of only 10 lines of really simple code.

It's clearly not that simple seeing how it has changed a lot between Node versions. Your version is also incorrect as it falls back to returning /tmp on Windows, where it should have returned \temp...

It's not about LOC. It's about delegating responsibility. This is the strength of node and npm. Small modules doing one thing well and people using it so they don't have to care how it works or reinvent the wheel incorrectly.

@sindresorhus sindresorhus deleted the ostmpdir-polyfill branch May 10, 2015 20:16
@raszi
Copy link
Owner

raszi commented May 11, 2015

Okay, you've convinced me. Unfortunately I could not cherry-pick your modifications because you've deleted that branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants