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

Fix Get/Set being enumerable #32264

Merged
merged 3 commits into from
Feb 27, 2020
Merged

Fix Get/Set being enumerable #32264

merged 3 commits into from
Feb 27, 2020

Conversation

pathurs
Copy link
Contributor

@pathurs pathurs commented Jul 5, 2019

Fixes #3610

@msftclas
Copy link

msftclas commented Jul 5, 2019

CLA assistant check
All CLA requirements met.

@pathurs
Copy link
Contributor Author

pathurs commented Jul 6, 2019

Note: I just followed the normal convention of setting enumerable: false, however it may be better to not set enumerable at all, which should default to false on it's own. (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperty)

@sheetalkamat Let me know what you think, seeing as you have already approved these changes.

@RyanCavanaugh
Copy link
Member

@sandersn let's bundle this in with the rest of the class field changes when we document what's happening

@pathurs can you fix the merge conflicts please? Sorry for the delay in getting this in

@sandersn sandersn added this to Gallery in Pall Mall Jan 30, 2020
@sandersn sandersn added the For Milestone Bug PRs that fix a bug with a specific milestone label Feb 1, 2020
@sandersn sandersn added this to Needs merge in PR Backlog Feb 27, 2020
@sandersn
Copy link
Member

I missed this in 3.7 when I was adding ES-compliant class field emit. However, I think it's better to put it into 3.9 now than to wait any longer.

@pathurs
Copy link
Contributor Author

pathurs commented Feb 27, 2020

@sandersn Thanks for merging from master, I don't know how I missed the notifications on this

@sandersn sandersn removed this from Gallery in Pall Mall Feb 27, 2020
@sandersn
Copy link
Member

Travis fails because it still thinks that we're using submodules for the user tests. Not sure why.

@sandersn sandersn merged commit 5c85feb into microsoft:master Feb 27, 2020
PR Backlog automation moved this from Needs merge to Done Feb 27, 2020
@sandersn
Copy link
Member

Thanks for your patience on this @pathurs

@pathurs pathurs deleted the getter-setter-non-enumerable branch February 27, 2020 22:13
@ntucker
Copy link

ntucker commented Mar 27, 2020

Does this affect the types for keyof T when T has getters?

@DanielRosenwasser
Copy link
Member

No, this affects emit, not the type system.

@benpetermorris
Copy link

This is a significant breaking change in our case. The UI library we use (Telerik's Kendo) uses property enumerability to convert "plain" objects to the Observable variant it uses within its UI classes.

With this change, property definitions are excluded from the conversion and the associated properties are no longer present on objects retrieved from Kendo's containers.

I support standards, of course, but it would be nice if there was an option to keep the old behavior. I'd be surprised if the new behavior is as innocuous as its low profile in the changelog seems to suggest.

@deltakosh
Copy link

deltakosh commented Jun 5, 2020

I agree with @benpetermorris. This has a massive implication for library using TS

@iMarv
Copy link

iMarv commented Aug 20, 2020

We have been struggling with this breaking change aswell as we were relying on being able to enumerate over getters.
Our solution now is to use the following decorator on all getters that shall be included when enumerating the objects:

function Enumerable(
    target: unknown,
    propertyKey: string,
    descriptor: PropertyDescriptor
) {
    descriptor.enumerable = true;
};
class Testi {
    @Enumerable
    get prop(): string {
        return 'moin';
    }
}

I guess this is a bit hacky, but the cleanest solution we could come up with.

OleksiiZubko added a commit to heremaps/here-data-sdk-typescript that referenced this pull request Oct 11, 2020
This CR updates Typescript to version 4.0.3,
@types/node to version 14.11.8,
ts-node to version 9.0.0

Also, because of changes in Typescript 3.9.0
(microsoft/TypeScript#32264)
the testing becoming failing.

The testing (sinon) library uses for stubbing dependencies
the enumerable properties of objects.

We do not able to use that way if we need to stub some dependency
in tests  with Typescript 3.9+ versions.

The right way is about refactoring code
(adding dependency injection, for example), but it's a huge task.

Fo resolving that problem, we use the 'tslib 1.13.0' helper module
and compile for testing the code with included helpers from that lib.

It's at least a working solution until a better way is found.

Resolves: OLPEDGE-2315

Signed-off-by: Oleksii Zubko <[email protected]>
OleksiiZubko added a commit to heremaps/here-data-sdk-typescript that referenced this pull request Oct 11, 2020
This CR updates Typescript to version 4.0.3,
@types/node to version 14.11.8,
ts-node to version 9.0.0

Also, because of changes in Typescript 3.9.0
(microsoft/TypeScript#32264)
the testing becoming failing.

The testing (sinon) library uses for stubbing dependencies
the enumerable properties of objects.

We do not able to use that way if we need to stub some dependency
in tests  with Typescript 3.9+ versions.

The right way is about refactoring code
(adding dependency injection, for example), but it's a huge task.

Fo resolving that problem, we use the 'tslib 1.13.0' helper module
and compile for testing the code with included helpers from that lib.

It's at least a working solution until a better way is found.

Resolves: OLPEDGE-2315

Signed-off-by: Oleksii Zubko <[email protected]>
OleksiiZubko added a commit to heremaps/here-data-sdk-typescript that referenced this pull request Oct 12, 2020
This CR updates Typescript to version 4.0.3,
@types/node to version 14.11.8,
ts-node to version 9.0.0

Also, because of changes in Typescript 3.9.0
(microsoft/TypeScript#32264)
the testing becoming failing.

The testing (sinon) library uses for stubbing dependencies
the enumerable properties of objects.

We do not able to use that way if we need to stub some dependency
in tests  with Typescript 3.9+ versions.

The right way is about refactoring code
(adding dependency injection, for example), but it's a huge task.

Fo resolving that problem, we use the 'tslib 1.13.0' helper module
and compile for testing the code with included helpers from that lib.

It's at least a working solution until a better way is found.

Resolves: OLPEDGE-2315

Signed-off-by: Oleksii Zubko <[email protected]>
mcdurdin added a commit to keymanapp/keyman that referenced this pull request May 10, 2022
Ref microsoft/TypeScript#32264 (comment)

We need getters for the ActiveLayout classes to be enumerable so that we
can copy them across in the polyfill functions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

Why are TS get/set definedProperties in a class transpiled as enumerable/writable?
10 participants