Using accessors makes code less clean and readable

[farwayer]

Hi!

I'm an author of several libraries that are actively used decorators. All of them are related to the declaration of data structures and validation. Some time ago I tried to port one of the lib to the new decorator system and found that it is not possible to define getter/setter of class field without using accessors.

Everyone know that readability is one of the key property of good quality code. And imo adding accessor modifier is step back from this point of view. Let me show.

Data struct definition with legacy decorators:

class News {
  @str id
  @str author
  @str title
  @str preview
  @str text
  @jsonDate createAt
  @jsonDate updatedAt
  @array(str) assets
}

With new decorators:

class News {
  @str accessor id
  @str accessor author
  @str accessor title
  @str accessor preview
  @str accessor text
  @jsonDate accessor createAt
  @jsonDate accessor updatedAt
  @array(str) accessor assets
}

As you can see, this structure is more difficult to read. And the type description is now separated from the field name. This is absolutely against what decorators should be intended for. Namely to make the code simpler and clearer. Not to mention the fact that while writing the code you now have to write two words instead of one.

And if the creator of the decorator forgets to check that the user used the accessor keyword, this can lead to unpleasant, hard-to-understand errors. Even the fact that decorator authors now need to specify the mandatory use of the accessor keyword in the documentation is no very good.

This issue is related to issue created by developer of two popular store libraries MobX and Mobx-State-Tree
#478

I would be happy to discuss this problem if it's not too late. Thanks!

[pzuraq]

@farwayer this issue has been discussed many many times before, both in threads on this repository and in plenary. The fact is that this proposal would not advance without this design choice, because implementers believed it would be impossible to optimize such shape changes. I spent quite a long time trying to find another way - over a year of alternative proposals. Each one was discussed and ultimately proved to be too complicated or unoptimizable.

[farwayer]

@pzuraq thanks for the answer!

This is my vision of the problems as an active user of decorators.

I look at the situation only from the point of view of the user of the language (albeit with a very large experience). I understand that there may be technical issues that affect the solution. But it seems to me that keeping the language clean, understandable and usable is very important.

[farwayer]

I'm not sure it is techically possible but even swaping accessor and decorator makes code a bit cleaner:

class News {
  accessor @str id
  accessor @str author
  accessor @str title
  accessor @str preview
  accessor @str text
  accessor @jsonDate createAt
  accessor @jsonDate updatedAt
  accessor @array(str) assets
}

[pzuraq]

But it seems to me that keeping the language clean, understandable and usable is very important.

Agreed. The committee has debated this form a lot, and believes that it is ultimately clean and understandable. Some even believe it is more clean and understandable, because you now know that the value being decorated is dynamic, it's an accessor.

I'm not sure it is techically possible but even swaping accessor and decorator makes code a bit cleaner:

I think this would be suboptimal for a few reasons:

1. It would lead to weird new lines in cases where you have multiple decorators on the same accessor.
2. It would not really be compatible with the grouped accessors proposal
3. It would lose the clarity mentioned above, that you are decorating a dynamic value.
4. It would break with other modifiers such as async on methods, which always come after decorators.

[farwayer]

I think you are right about the sequence. I gave only a single case when it would be better.

And about the fact that accessor with decorator looks more understandable, I would not say. The use of decorators in itself implies modifications to the field, the meaning of which can only be fully understood by examining its code.

[pzuraq]

True, but you do gain some information about the nature of the decoration from the usage. For instance:

class Foo {
  @inject foo;
  @inject accessor bar;
}

Just from reading this code, we can know that foo will be injected on initialization of the class, because it is a class field. Access is not being modified, so it must be an immediate injection, not a lazy one. bar, on the other hand, looks like it could be lazy - it could be looked up on first access later on.

This is definitely debatable, I would not say that this is a perfect reason for adding accessor on its own. The biggest reason was performance overall, which was blocking the proposal from moving forward.

[pzuraq]

Also worth noting, this type of change is not typically one that can occur in stage 3 without a very good reason. From https://tc39.es/process-document/

Limited: only those deemed critical based on implementation experience

Changes based on readability typically don't reach that level of critical, especially when they were previously discussed and decided on.

[farwayer]

I would not say that this is a perfect reason for adding accessor on its own

You are right. But if it's related to performance, then it's a more important reason. Although I would prefer readability.

Also worth noting, this type of change is not typically one that can occur in stage 3 without a very good reason.

Yeah, changing something on Stage 3 is not easy... Anyway @pzuraq thanks for letting know what's going on.

[farwayer]

Just found accessor modifier makes field non-enumerable (what is expected if it is replacing private property with setter and getter).

It turns out that with the new decorators, the ability to add a setter / getter leaving the field enumerable will disappear. I'm right? It's sad if so.

[pzuraq]

@farwayer this was also true with all other forms of decorators. They become non-enumerable because you move them to a getter/setter on the prototype. That had always been the case, even with Babel Legacy/TS experimental decorators.

[farwayer]

With legacy the code will print prop:

let alwaysOne = () => {
  return {
    enumerable: true,
    get() {
      return 1
    }
  }
}

class Test {
  @alwaysOne prop
}

let test = new Test()
for (let p in test) {
  console.log(p)
}

But with accessor and new decorators no:

let alwaysOne = () => {
  return {
    get() {
      return 1
    },
  }
}

class Test {
  @alwaysOne accessor prop
}

let test = new Test()
for (let p in test) {
  console.log(p)
}

[pzuraq]

Ah, so you mean enumerability on the prototype itself. Yes, that was another hard constraint from engine implementers - no changes to enumerability, writability, or configurability. These were also blocked due to performance constraints, as doing so would change the shape of the class dynamically, which prevents JIT optimizations.

The future solution that I think will be considered is keywords which would be applied to the properties directly, e.g. readable, enumerable etc.

[mhofman]

Defaulting the enumerability of an auto-accessor to non-enumerable seem like a footgun. I think most people would have the expectation those can be used by consumer of the instances the same way as fields. If this an intentional choice, where was the decision recorded so I can familiarize myself with the arguments?

[pzuraq]

@mhofman the logic is that it is effectively sugar for a standard get and set style method, so it should work exactly the same. It was an intentional choice to preserve this desugaring.

[pzuraq]

Related, I've heard from some members on the committee that in general iterating via for in should not be encouraged, with for (let key of Object.keys(foo)) being preferred. I don't remember the exact reason this was the case, but it was a point of contention in the design of metadata (which relies on it because it uses prototypical inheritance a lot) and part of the reason metadata is going to have a null prototype.

[mhofman]

Oh yeah prototype properties wouldn't be enumerated except by for..in, so I guess we already lost at that point.

[ljharb]

for-in has been widely considered a bad practice for decades across the entire community, precisely because it enumerates inherited properties - the encouraged practice is to use the own property enumerators (Object.keys, Object.values, Object.entries, Object.getOwnPropertyNames, Object.getOwnPropertySymbols, Reflect.ownKeys) recursively with Object.getPrototypeOf if that's something that's desired. It would be very unfortunate if TC39 tacitly or explicitly endorsed a decades-old bad practice as a means to solve any use case.

[farwayer]

for-in has been widely considered a bad practice

There is nothing wrong with for..in. Its behavior is described in thousands of guides and most of those who write in the language know this feature. Useful example for for..in is data-only classes with inheritance and validation by decorators.

class User {
  @str username
  @str email
}

class AuthenticatedUser extends User {
  @str authToken
}

Using Object.keys with Object.getPrototypeOf recursively will surely complicate the code.

[trusktr]

The workaround is easy!

@classDeco
class Mine {
  @fieldDeco foo = 123 // Look ma, no accessor!
}

Details here:

#514 (comment)

In practice I use it like this:

@reactive
class Mine {
  @signal count = 0
}

const obj = new Mine()

setInterval(() => {
  obj.count++ // triggers reactivity 
}, 1000)

createEffect(() => {
  // This logs the new value every second.
  console.log('count:', el.count)
})

Your class will become:

@deco
class News {
  @str id
  @str author
  @str title
  @str preview
  @str text
  @jsonDate createAt
  @jsonDate updatedAt
  @array(str) assets
}

I too felt that accessor was overly verbose to repeat everywhere, so I skipped it, and haven't had any issues!

[pzuraq]

@trusktr note that doing this will result in worse performance across the board, I wouldn't recommend it. Any class that does this will not be able to monomorphize decorated property access, meaning it will never be optimized.

Edit: Hmm, actually because you're installing it on the prototype and not the instance it should be able to monomorphize, but it will still have worse performance as you're likely backing the property with a weakmap or installing class properties on the instance which will result in worse instantiation time.

[pzuraq]

Closing this issue because I believe it's been discussed fully, the accessor keyword is additional syntax overhead, but it also encodes information that is useful to both developers and compilers.

[trusktr]

@trusktr note that doing this will result in worse performance across the board, I wouldn't recommend it

I've been using this in smooth 3D WebGL applications with no issues. There may be some edge cases where it matters (like you have 100,000 objects to animate at the same time or something), but for most people it won't matter. accessor is basically a micro optimization in practice as far as things I've been making.

Plus with technology advancing, "today's" mobile devices can likely run the slower version faster than "yesterday's" desktops can run the faster version. (f.e. I can run Doom 3 at full speed inside a browser in WebAssembly on my phone).

If there's a real reason to switch in practice, for some scenario, then I will, but until then the DX is nicer without accessor on all properties.