GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Fix Get/Set being enumerable by pathurs · Pull Request #32264 · microsoft/TypeScript

Conversation

@pathurs

@pathurs Patrick Stanford (pathurs) commented Jul 5, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Fixes #3610

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
SlurpTheo and weslenng reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
Patrick Stanford (pathurs) added 2 commits July 5, 2019 19:30
@pathurs
Fix Get/Set being enumerable
3626fed 
fixes #3610
@pathurs
fix tests
83252fe
@msftclas

Microsoft Contribution License Agreements (msftclas) commented Jul 5, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sheetalkamat
Sheetal Nandi (sheetalkamat) approved these changes Jul 5, 2019
View reviewed changes
@pathurs

Patrick Stanford (pathurs) commented Jul 6, 2019

Copy link
Copy Markdown
Contributor Author

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)

Sheetal Nandi (@sheetalkamat) Let me know what you think, seeing as you have already approved these changes.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RyanCavanaugh Ryan Cavanaugh (RyanCavanaugh) assigned Daniel Rosenwasser (DanielRosenwasser) and Nathan Shively-Sanders (sandersn) and unassigned Daniel Rosenwasser (DanielRosenwasser) Aug 15, 2019
@RyanCavanaugh

Ryan Cavanaugh (RyanCavanaugh) commented Aug 15, 2019

Copy link
Copy Markdown
Member

Nathan Shively-Sanders (@sandersn) let's bundle this in with the rest of the class field changes when we document what's happening

Patrick Stanford (@pathurs) can you fix the merge conflicts please? Sorry for the delay in getting this in
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sandersn Nathan Shively-Sanders (sandersn) added the For Milestone Bug PRs that fix a bug with a specific milestone label Feb 1, 2020
@sandersn
Merge branch 'master' into getter-setter-non-enumerable
ae78ad6
@sandersn

Nathan Shively-Sanders (sandersn) commented Feb 27, 2020

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pathurs

Patrick Stanford (pathurs) commented Feb 27, 2020

Copy link
Copy Markdown
Contributor Author

Nathan Shively-Sanders (@sandersn) Thanks for merging from master, I don't know how I missed the notifications on this
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sandersn

Nathan Shively-Sanders (sandersn) commented Feb 27, 2020

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sandersn
Nathan Shively-Sanders (sandersn) merged commit 5c85feb into microsoft:master Feb 27, 2020
@sandersn

Nathan Shively-Sanders (sandersn) commented Feb 27, 2020

Copy link
Copy Markdown
Member

Thanks for your patience on this Patrick Stanford (@pathurs)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pathurs
Patrick Stanford (pathurs) deleted the getter-setter-non-enumerable branch February 27, 2020 22:13
@ntucker

Nathaniel Tucker (ntucker) commented Mar 27, 2020

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Mar 27, 2020

Copy link
Copy Markdown
Member

No, this affects emit, not the type system.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced May 14, 2020
Sinon stubbing failed after upgrade to 3.9.2 #38568
Closed
Spying on ES modules with typescript > 3.9 jasmine/jasmine#1817
Closed
@benpetermorris

benpetermorris commented May 29, 2020

Copy link
Copy Markdown

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.
antonblock, TheWizz, sebavan, bghgary, Taytay, JabX, danielkcz, ivangonzalezsp, EmandM, Lambik, and 7 more reacted with thumbs up emoji
All reactions
  • 👍 17 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AviVahl Avi Vahl (AviVahl) mentioned this pull request Jun 5, 2020
Backward compat issue with non enumerable properties #38958
Closed
@deltakosh

David Catuhe (deltakosh) commented Jun 5, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

I agree with benpetermorris. This has a massive implication for library using TS
GregKosmo reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JabX Damien Frikha (JabX) mentioned this pull request Jun 28, 2020
Typescript 3.9 breaks getters in extendObservable mobxjs/mobx#2386
Closed
@veselints Veselin Tsvetanov (veselints) mentioned this pull request Aug 17, 2020
Kendo Observable is missing properties in TypeScript 3.9 telerik/kendo-ui-core#5973
Closed
@iMarv

Marvin Altemeier (iMarv) commented Aug 20, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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.
GregKosmo, jcroll, sinedied, elliott-beach, wave52, mcdurdin, and my5201314zwl reacted with thumbs up emoji
All reactions
  • 👍 7 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Oleksii Zubko (OleksiiZubko) added a commit to heremaps/here-data-sdk-typescript that referenced this pull request Oct 11, 2020
@OleksiiZubko
Update Typescript.
9c64360 
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 Oleksii Zubko (OleksiiZubko) mentioned this pull request Oct 11, 2020
Update Typescript. heremaps/here-data-sdk-typescript#382
Merged
Oleksii Zubko (OleksiiZubko) added a commit to heremaps/here-data-sdk-typescript that referenced this pull request Oct 11, 2020
@OleksiiZubko
Update Typescript.
c4c6134 
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]>
Oleksii Zubko (OleksiiZubko) added a commit to heremaps/here-data-sdk-typescript that referenced this pull request Oct 12, 2020
@OleksiiZubko
Update Typescript.
9cd32c2 
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]>
@jacobg jacobg mentioned this pull request Nov 17, 2020
mocking imports broken by typescript 3.9 EmandM/ts-mock-imports#32
Closed
@michkot michkot mentioned this pull request Feb 26, 2021
Why are TS get/set definedProperties in a class transpiled as enumerable/writable? #3610
Closed
Marc Durdin (mcdurdin) added a commit to keymanapp/keyman that referenced this pull request May 10, 2022
@mcdurdin
chore(web): make getters enumerable
907d4ac 
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.
@mcdurdin Marc Durdin (mcdurdin) mentioned this pull request May 10, 2022
chore(web): fixup keyboard-processor and input-processor tests 🎡 keymanapp/keyman#6597
Merged
@github-actions github-actions Bot mentioned this pull request Aug 14, 2022
Remove span tag in title and unnecessary TOC in Ko microsoft/TypeScript-Website-Localizations#164
Merged
@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sheetalkamat Sheetal Nandi (sheetalkamat) sheetalkamat approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@sandersn Nathan Shively-Sanders (sandersn)

Labels

For Milestone Bug PRs that fix a bug with a specific milestone

Projects

PR Backlog
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

Uh oh!

There was an error while loading. Please reload this page.

10 participants

@pathurs @msftclas @RyanCavanaugh @sandersn @ntucker @DanielRosenwasser @benpetermorris @deltakosh @iMarv @sheetalkamat
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.