(fix) Class fields are set to undefined if there's no initializer

[williamtetlow]

Description:
Fixes #2117 by returning null instead of void 0 for an initializer that is not set on property descriptor. See this comment for good description of problem

This caused tests to fail for #2127 but I have provided alternative fix through checking for declare modifier on fields that shouldn't be emitted.

TS ESNext now emits class fields with no initializer by default. See useDefineForClassFields

class AClass {
  field: String
}

// emits (pre-esnext)
class AClass {}

// emits (esnext)
class AClass {
  field
}

It supports overriding this behaviour through the declare modifier.

class AClass {
  declare field: String
}

// emits (esnext)
class AClass {}

#960 and #2127 should now be solved by using declare modifier. Swc will emit class fields by default now to match tsc behaviour.

BREAKING CHANGE:
#960
#2127

By default, class fields will be defined and set to undefined breaking the two issues above.

Fields should have declare modifier if they shouldn't be emitted.

class TestClass2 {
  @deco public declare testProperty: Date;
}

Related issue (if exists):
#2117
#2127
#960

[CLAassistant]

All committers have signed the CLA.

[kdy1]

If I remeber correctly, this is wrong

[williamtetlow]

@kdy1 I had a look into this issue and how to fix it while keeping fix for #2127

See 46d4cd8

For #2127 I compiled the same code with SWC, Babel, esbuild and TSC.

SWC - ✅
Babel - ❌
TSC (esnext) - ❌
TSC (<esnext) - ✅
esbuild - ❌

Looks like only SWC and TSC (lower than esnext) supports this at the moment all the others return undefined for the property.

I read the spec to see if there was a mention of how this should be handled (e.g. no initializer always sets value to undefined) but there wasn't anything.

However, I found an alternative way to fix #2127 and moved it to _initializerDefineProperty.

If there is no initializer for the property and the property already has a value that != void 0 then it will set it to the current value. Otherwise, set it to void 0.

If object does not have own property it will check the prototype chain.

[williamtetlow]

I've pushed my local test bench for this issue which shows how each tool handles both issues https://github.com/williamtetlow/2117-2127-swc-testbench

For #2127 the reason this code worked for the issue raiser on ts-jest is because ts-jest uses TSC and they were using es5 which supports #2127 but does not support #2117

[kdy1]

Oh... Then I think we should follow esnext behavior of tsc.

[williamtetlow]

It looks like it's related to useDefineForClassFields which is for migrating to new TC39 standard version of class fields.

This is turned on by default for esnext.

The issue in #2127 is that the field is defined on prototype.constructor but by defining the field on the class too the original is hidden.

((_class = class TestClass2 {
    constructor() {
      // defining here hides prototype.constructor.testProperty
      _initializerDefineProperty(this, "testProperty", _descriptor, this);
    }
  })

To help mitigate the second issue, you can either add an explicit initializer or add a declare modifier to indicate that a property should have no emit.

Going forward tsc will define class fields by default (like swc is doing) but you can override this and not emit by adding declare modifier to a property.

class TestClass2 {
  @deco public declare testProperty: Date;
}

I've added this logic to swc in 813e3eb#diff-8f03d343376a656cd03aec743c8cb8b60d7c1a4de29f48d940be0795c0931235R676

[williamtetlow]

I'm not sure if there is a way to opt out of this logic for Ecmascript unless we make a config setting.

Should we add setting or are we ok with making #2127 and #960 not work anymore when compiling Ecmascript to better replicate Typescript behaviour?

We could add setting to jsc.transform and then we can allow disabling emit logic for Typescript too

[kdy1]

Should we add setting or are we ok with making #2127 and #960 not work anymore when compiling Ecmascript to better replicate Typescript behaviour?

I think it's fine, and it's the right way

[kdy1]

Is this PR ready for review/merge?

[williamtetlow]

@kdy1 yep it's ready 😄

[kdy1]

Thanks!

[aboqasem]

When could this fix land in Next.js?

[kdy1]

@aboqasem I'll make a PR today.

[aboqasem]

@kdy1 Thank you for your effort!

[JCMais]

Did this land on Next.js?