file.path is only a getter

[jonschlinkert]

Version

23

Platform

This is going to break every library I have that uses `fs.readdir` with `withFileTypes`:


      file.path = path.join(dir, file.name);
               ^
TypeError: Cannot set property path of #<Dirent> which has only a getter


The `file.path` property is everywhere in my code, it represents `path.join(dir, file.name)`, which is the full path to the file. This is how millions of projects are using that property already, and it's decorated (in my code) onto the file object returned by `withFileTypes`.

Subsystem

No response

What steps will reproduce the bug?

fs.readdirSync(__dirname).map(f => {
  f.path = path.join(__dirname, f.name)
})

How often does it reproduce? Is there a required condition?

Since withFileTypes has been around for years, this will happen with every library that I created that uses withFileTypes.

What is the expected behavior? Why is that the expected behavior?

file.path should be the absolute path to the file if it's going to be decorated, since this is a well established convention for every file and path library I've ever seen.

What do you see instead?

I see an error with a read only property, and a warning that path is deprecated. I have to say this is completely absurd. parentPath? Since node has existed dirname has been the name of directories.

Why is someone deciding to name it parentPath? Who made this decision? Is there a precedent for this or a spec you can point to? FWIW I've created hundreds of path libraries and I have never seen that term used anywhere.

Additional information

I think whoever is making these decisions should be involving people with more experience in Node.js. These decisions are going to cause massive headaches for existing code that use firmly establish conventions that have existed for years.

If you're going to finally fix file/dirent objects to be more useful, why not just use conventions of tooling that has already proliferated in the ecosystem, like vinyl files? I'm really interested in the reasoning here.

[RedYetiDev]

This is by-design. dirent.path is deprecated, and according to the documentation, it is a read-only property.

https://nodejs.org/api/fs.html#direntpath states:

Alias for dirent.parentPath. Read-only.

[RedYetiDev]

Sorry for the close/reopen, 😅 butter-fingers...

---

I think whoever is making these decisions should be involving people with more experience in Node.js.

The PR to (runtime) deprecate this (#51050) was open for over five months before merging. There was plenty of opportunity for feedback during that time. You can always open a PR requesting a change (though it may have to wait until after the deprecation cycle).

If you're going to finally fix file/dirent objects to be more useful

If you have advice for it to be more useful, I'm sure you can open a feature request.

---

Additionally, your issue isn't very clear, which of the following are you asking:

1. Why is it only a getter?

The property is defined as read-only. This is not a bug.

3. Why was it deprecated?

Feel free to search past issues, such as #50976 for more context

5. Why isn't it XYZ?

If you'd like to see a change, open a feature request or PR.

---

Assuming that the intent of this issue was (1) Why is it only a getter, I've marked this was wontfix, as the getter is by design.

[jonschlinkert]

The property is defined as read-only. This is not a bug.

It should be. My packages represent about 10% of the downloads in Node.js and this will break my packages.

[marco-ippolito]

cc @aduh95

[aduh95]

You can do Object.defineProperty() to not hit the getter error. If you're interested in the history of .path and .parentPath on Dirent class, you can find the context in the PRs that introduced them.

[jonschlinkert]

You can do Object.defineProperty() to not hit the getter error. If you're interested in the history of .path and .parentPath on Dirent class, you can find the context in the PRs that introduced them.

Thanks for explaining how javascript properties work. I'm not interested in the discussions at all. If I read the discussions, will I find invitations to people like me, @doowb, @phated, or other people who have created the most used path/file related projects in Node.js? I don't remember receiving any invites to the discussion, or any discussion about similar topics, yet my packages account for almost 10% of Node.js total downloads, and some of my packages have more than 30 million dependents. IMHO that makes no sense at all, but I can live it it.

What I am interested in, is not having to deal with my already existing packages breaking when people start using the latest version(s) of Node.

A simple search shows that 148,000 javascript files, and 53,000 typescript files on GitHub use the exactly term: file.path. At least 1,000 of those are directly using withFileTypes in that specific file, and surely many of those references are also using Dirent but not directly in that file. More importantly, there are countless other ways to define that property: f, dirent etc etc. so it's likely that there are many other files doing the same.

Why is this even a debate. Node should have been using symbols or something if you want the ability to arbitrarily change properties. This is unacceptable. Please either make path writable or revert this change. You're not the one that has to deal with the consequences of this change. This was a bad decision, and it needs to be reverted. You can't just make a very common property read-only on an object that users have been able to decorate for years.

[mcollina]

I don't remember receiving any invites to the discussion, or any discussion about similar topics, yet my packages account for almost 10% of Node.js total downloads, and some of my packages have more than 30 million dependents. IMHO that makes no sense at all, but I can live it it.

I can see why you are upset, and I have been similarly been upset in the past. The only way to be sure to be part of the discussion is to show up, and devoting effort and care to the project. It still happens from time to time to me as well that a change that affects my projects badly gets landed, and I’m on the TSC. The only solution is to show up, and this is on each of us.

What I propose:

1. we revert the breakage for v23
2. you send a few of your modules to be included in CITGM, so we can check them before every release (this has some responsibility on your end
3. we assess if we still want to make this deprecation and how.

[mcollina]

@jonschlinkert, which modules did it break?

[mcollina]

I looked at it a bit more in detail, and it seems the biggest issue is that it was made read-only, and on second instance its about the deprecation itself.

Therefore, a quick solution to this problem is to make the property writable in https://github.com/nodejs/node/pull/51050/files#diff-5cd422a9a64bc2c1275d70ba5dcafbc5ea55274432fffc5e4159126007dc4894R228.

@aduh95 I don't see any reasoning in that PR on why it was made read-only, so I guess this should be non-contentious.

I would also mark dirent.path as a "pending deprecation" instead and living it there for a few more cycles. We did it in the past for other popular properties.

[aduh95]

I would also mark dirent.path as a "pending deprecation" instead and living it there for a few more cycles

On the contrary, I would move it to EoL so it no longer affects userland modules

[RedYetiDev]

I would move it to EoL so it no longer affects userland modules

Agreed, however that would need to be a semver-major change per the deprecation cycle, so it'd half to wait 6 months.

[jonschlinkert]

I can see why you are upset, and I have been similarly been upset in the past.

Yeah sorry if I came across harshly.

The only solution is to show up, and this is on each of us.

Fair

@jonschlinkert, which modules did it break?

I was saying it would break them when people start using the latest. Fortunately I haven't gotten issues yet, but I've used file.path pretty much every time I've used withFileTypes. Admittedly I have a terrible memory lately and I can't remember which packages I used it in. I just know that I did, and I'm not looking forward to the issues from this lol.

it seems the biggest issue is that it was made read-only, and on second instance its about the deprecation itself.

Agreed. The read-only decision is my only concern.

I like your proposed solution(s).

[mcollina]

I will send a PR as early as I can (if no one else beat me to it.

[LiviaMedeiros]

I would highly recommend to see the root cause rather than focusing on the property being non-writable and potentially making it worse with quick solutions.

The value of .path property was changed and the change got into release lines. The v18.x hasn't reach EOL yet, and in these versions .path pointed on the file rather than on path to its parent directory.
This is unfortunate, but I hope we all can agree that .path is not appropriate name for path to dirent's parent, and that having a property with ambiguous value between releases is simply dangerous.

For the code examples, consider the options.recursive:

-  fs.readdirSync(__dirname).map(f => {
-    f.path = path.join(__dirname, f.name)
+  fs.readdirSync(__dirname, { recursive: true }).map(f => {
+    f.absolutePath = path.resolve(__dirname, f.parentPath, f.name)
  })

file.path should be the absolute path to the file if it's going to be decorated, since this is a well established convention for every file and path library I've ever seen.

IIRC dirent.path never was an absolute path, right now it has two possible values in Node.js and is being deprecated. I can see why you would want absolute path here, but overwriting an existing property of Dirent instance with value of third meaning feels questionable. Can we see a comprehensive example that shows why the library decorates it this way? Maybe it would benefit if we add desired values on Dirent instances by default on Node.js side?

Bikeshedding .parentPath name can also be considered, maybe we can have .dirname alias. But this probably should be a separate issue.

I was saying it would break them when people start using the latest. Fortunately I haven't gotten issues yet, but I've used file.path pretty much every time I've used withFileTypes. Admittedly I have a terrible memory lately and I can't remember which packages I used it in. I just know that I did, and I'm not looking forward to the issues from this lol.

Decorating is okay but if you use the original dirent.path value returned by Node.js, it will most likely cause bugs. Please migrate to .parentPath and doublecheck that it's assumed to be relative path to dirent's dirname rather than path to dirent itself.

---

To solve the current issue with runtime deprecation implementation: would defining a setter solve it? Here's the property definition:

node/lib/internal/fs/utils.js

Lines 212 to 219 in 0668e64

	ObjectDefineProperty(Dirent.prototype, 'path', {
	__proto__: null,
	get: deprecate(function() {
	return this.parentPath;
	}, 'dirent.path is deprecated in favor of dirent.parentPath', 'DEP0178'),
	configurable: true,
	enumerable: false,
	});

I think, we can assign a private Symbol by default and define a setter. If the value got overwritten, .path should return the new value (without deprecation notice?). If it wasn't overwritten, we print deprecation notice and return .parentPath value. Does this solution have downsides?

As for future of this property, DEP0178 was added almost an year ago, I think we should remove this property in v24.x, even if there is no good workaround and we have to skip runtime deprecation.
In this case, it should be announced clearly so folks can notice it and migrate ASAP.

[aduh95]

even if there is no good workaround and we have to skip runtime deprecation.

It is already runtime deprecated, and the workaround (.parentPath) exists.

IIRC dirent.path never was an absolute path

It’s an absolute path if an absolute path or file: URL was passed to readdir.

[tomweptinstall]

It should be. My packages represent about 10% of the downloads in Node.js and this will break my packages.

I was saying it would break them when people start using the latest. Fortunately I haven't gotten issues yet

First of all I truly love your work, but when you make claims like this, it diminishes some of that respect and credibility, at least in my opinion.

I think whoever is making these decisions should be involving people with more experience in Node.js.

This is also an intriguing point: who is actually more qualified to be experienced, and who can confidently make that claim?

[jonschlinkert]

First of all I truly love your work, but when you make claims like this, it diminishes some of that respect and credibility, at least in my opinion.

My point was that this wasn't a theoretical scenario and that I have first-hand knowledge that this will break some of my packages. And given the number of other projects that depend on my packages, this breakage should be considered a bug. If you don't use real world usage or regressions to decide what is a bug and what isn't, then what is your yardstick? If you took that statement to mean something else, that was not my intent.

I can see how you might have thought I intended my comment differently, but that's not where my mind was.

[doowb]

@tomweptinstall

First of all I truly love your work, but when you make claims like this, it diminishes some of that respect and credibility, at least in my opinion.

FWIW I know @jonschlinkert really well and he didn't mean for his comment to come across the way you may be taking it.

[mcollina]

This is fixed and will be released in the next v23 release

[jonschlinkert]

thank you!!!