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

Decorators in 4.3.x can't use Object.defineProperty anymore. #44673

Closed
dannysmc95 opened this issue Jun 20, 2021 · 9 comments
Closed

Decorators in 4.3.x can't use Object.defineProperty anymore. #44673

dannysmc95 opened this issue Jun 20, 2021 · 9 comments

Comments

@dannysmc95
Copy link

Bug Report

🔎 Search Terms

  • decorator broken
  • decorator defineProperty
  • decorator Object.defineProperty
  • decorator target issue

🕗 Version & Regression Information

Issue starts in 4.3.x (was previously fine in 4.2.4 and below).

  • This changed between versions 4.2.4 and 4.3.x

⏯ Playground Link

The playground link with this code works fine, so it's not a reliable test, but when using the NPM version of typescript that is where the issue is, see my gist here:
https://gist.github.com/dannysmc95/da689ae30ef6c05e3fb1778a7c8143a8

If you create a basic file with typescript 4.2.4 you will see the file works, if you then install a 4.3.x version (I did a few versions including latest and next) it will say:

Cannot read property 'say' of undefined

💻 Code

function SetValue(): any {
	return (target: Object, propertyKey: string) => {
		Object.defineProperty(target, propertyKey, {
			get: () => {
				return new MyHelper();
			},
		});
	}
}

class MyHelper {
	public say(message: string): void {
		console.log(message);
	}
}

class MyClass {

	@SetValue() public helper!: MyHelper;
}

const instance = new MyClass();
console.log(instance.helper.say('hello'));

🙁 Actual behavior

In versions above 4.2.4 (i.e. 4.3.x) this will throw the error:

TypeError: Cannot read property 'say' of undefined

In versions 2.4.2 and below it will output:

hello
undefined

That is correct.

🙂 Expected behavior

4.3.x should work the same as 4.2.4.

@MartinJohns
Copy link
Contributor

Seems to be a duplicate of #44449. Search terms: defineproperty decorator.

@dannysmc95
Copy link
Author

@MartinJohns thank you, my bad.

@dannysmc95
Copy link
Author

@MartinJohns based on the response from the linked bug, I see no way around this? How do I fix or work around the issue?

I can't define anything that makes it work? So what is the expectation, I looked at the linked proposal but I can't say I understand what it is saying, how do I use a property decorator to change the actual property? or is this now not functional, and things like dependency injection (that is where I found this bug) do not work anymore?

Example the actual code I am working with does:

export class SomeModule {

    @Inject() helper!: SomeHelper;

    public something(): void {
        console.log(this.helper.someMethod());
    }
}

@dannysmc95
Copy link
Author

dannysmc95 commented Jun 20, 2021

Sorry, to expand, I got it working by specifically defining: useDefineForClassFields as false, as that should be the default, but it looks like the default value is actually true for it.

https://www.typescriptlang.org/tsconfig#useDefineForClassFields

@MartinJohns
Copy link
Contributor

The default for useDefineForClassFields is false, unless you target esnext (in which case you should expect ECMAScript compliant behavior by default).

@dannysmc95
Copy link
Author

@MartinJohns but with ECMAScript compliant behaviour, what is the alternative solution? What is the point of property decorators?

@MartinJohns
Copy link
Contributor

The alternative solution is to wait for the decorator proposal to be finished and wait for it to be in stage 3 (and then wait for TypeScript to support it). The current decorator implementation in TypeScript is based on an old deprecated proposal, and the team is waiting for the proposal to be completed (and in stage 3) before investing any more effort in the decorator topic.

Remember that decorators are an experimental feature.

@dannysmc95
Copy link
Author

@MartinJohns understood.

@Glandos
Copy link

Glandos commented Jun 23, 2021

The default for useDefineForClassFields is false, unless you target esnext (in which case you should expect ECMAScript compliant behavior by default).

This wasn't mentioned in any release notes as noted by @CarterLi in #34787 (comment)

I guess that a new entry should be done to explicitly define the current situation. Either use esnext without decorators, or don't use esnext, or set useDefineForClassFields to false in the meantime.

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

No branches or pull requests

3 participants