[helpers TS conversion] reflection #16507

coelhucas · 2024-05-17T15:17:55Z

Q	A
Fixed Issues?	Part of #16500
Patch: Bug Fix?	👍
Major: Breaking Change?	No
Minor: New Feature?	No
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?	No
License	MIT

Migrate all Reflection helpers to TypeScript, following the specifications. This introduces changes from yarn gulp generate-runtime-helpers

babel-bot · 2024-05-17T15:19:57Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/56984

coelhucas · 2024-05-17T15:32:20Z

packages/babel-helpers/src/helpers/get.ts

+type Object = {
+  __proto__?: any;
+} & { [key: string]: unknown };
+


I re-used this bit in some places rather than extending object since __proto__ isn't "official", should I remove the usage of __proto__ instead?

That's not how to properly type object IMHO. I'd rather stick with (as any).__proto__ than declaring a type with a name Object.

coelhucas · 2024-05-17T15:32:45Z

packages/babel-helpers/src/helpers/getPrototypeOf.ts

+  // @ts-expect-error explicitly assign to function
+  _getPrototypeOf = Object.setPrototypeOf
+    ? // @ts-expect-error -- intentionally omitting the argument
+      Object.getPrototypeOf.bind(/* undefined */)


"Stole" these bits from #16505 approach

coelhucas · 2024-05-17T15:34:13Z

packages/babel-helpers/src/helpers/defineProperty.ts

+
+export default function _defineProperty<T extends Object>(
+  obj: T,
+  key: PropertyKey,


Just saw the comment about using string | symbol. Will fix it here

defineProperty is different since defineProperty does allow number to be a valid object key. Using string | symbol is mistyping.

JLHwung · 2024-05-17T15:38:12Z

packages/babel-helpers/src/helpers/defineEnumerableProperties.ts

-    for (var i = 0; i < objectSymbols.length; i++) {
-      var sym = objectSymbols[i];
+    var objectSymbols: Array<Symbol> = Object.getOwnPropertySymbols(descs);
+    for (const sym of objectSymbols) {


The babel 7 helpers should be ES5-compliant, so we should stick to the good old for loop.

nicolo-ribaudo · 2024-05-17T16:28:27Z

packages/babel-helpers/src/helpers/get.ts

+  this: unknown,
+  ...args: [target: T, property: P, receiver?: unknown]
+): P extends keyof T ? T[P] : any {
+  let _getImpl = _get;


Let's keep the original implementation where we reassign _get, and add a @ts-expect-error to it.

This is different from the original one because the feature detection would run every single time _get is called, rather than just the fist one.

Also for the other helpers.

nicolo-ribaudo · 2024-05-17T16:31:42Z

packages/babel-helpers/src/helpers/getPrototypeOf.ts

+    ? // @ts-expect-error -- intentionally omitting the argument
+      Object.getPrototypeOf.bind(/* undefined */)
+    : function _getPrototypeOf<T extends Object>(o: T) {
+        return o.__proto__ || Object.getPrototypeOf(o);


Instead of defining Object with __proto__, we can use o["__proto__"] and TS should stop complaining.

Ad an alternative, we can use

declare global { interface Object { __proto__: object; } }

and then TS will automatically let us use __proto__ on things of type object (with the lowercase o)

Merging with interface Object is bad. If we do this, TypeScript will add __proto__ to every object out there. Let's just use (as any).__proto__, it is better than __proto__ goes rouge.

nicolo-ribaudo · 2024-05-17T16:32:23Z

packages/babel-helpers/src/helpers/set.ts

+  property: string | symbol,
+  value: any,
+  receiver?: any,
+  isStrict: boolean = false,


Suggested change

isStrict: boolean = false,

isStrict?: boolean,

This file needs to be compatible with old browsers, so let's not use default params :)

SukkaW · 2024-05-17T16:30:31Z

packages/babel-helpers/src/helpers/getPrototypeOf.ts

+  __proto__?: any;
+} & { [key: string]: unknown };
+
+export default function _getPrototypeOf<T extends Object>(o: T) {


You don't need a <T> here. Object.getPrototypeOf doesn't require a generic.

SukkaW · 2024-05-17T16:30:58Z

packages/babel-helpers/src/helpers/objectSpread.ts

+  __proto__?: any;
+} & { [key: string]: unknown };
+
+export default function _objectSpread<T extends Object, U extends unknown[]>(


Normal object just works, don't introduce Object here.

SukkaW · 2024-05-17T16:32:08Z

packages/babel-helpers/src/helpers/set.ts

-        if (desc.set) {
+        if (desc && desc.set) {
          desc.set.call(receiver, value);
          return true;
-        } else if (!desc.writable) {
+        } else if (!desc || !desc.writable) {


You don't need to guard desc here. desc will always be defined, this is an implementation detail (about where and when set will ever be called), just use !.

SukkaW · 2024-05-17T16:32:45Z

packages/babel-helpers/src/helpers/set.ts

+  property: string | symbol,
+  value: any,
+  receiver?: any,
+  isStrict: boolean = false,


Do not use default parameters. Not only does it break the compatibility, it also increases the bundle size.

SukkaW · 2024-05-17T16:33:52Z

packages/babel-helpers/src/helpers/superPropBase.ts

+
+export default function _superPropBase(
+  object: Object,
+  property: string | symbol,


Use PropertyKey here. You are typing arguments, not typing declared variables.

SukkaW · 2024-05-17T16:41:41Z

packages/babel-helpers/src/helpers/set.ts

+
+function set(
+  target: Object,
+  property: string | symbol,


And PropertyKey here as well.

nicolo-ribaudo · 2024-05-17T18:04:14Z

packages/babel-helpers/src/helpers-generated.ts

  get: helper(
    "7.0.0-beta.0",
-    'import superPropBase from"superPropBase";export default function _get(){return _get="undefined"!=typeof Reflect&&Reflect.get?Reflect.get.bind():function(e,t,r){var p=superPropBase(e,t);if(p){var n=Object.getOwnPropertyDescriptor(p,t);return n.get?n.get.call(arguments.length<3?e:r):n.value}},_get.apply(this,arguments)}',
+    'import _superPropBase from"superPropBase";export default function _get(){return _get="undefined"!=typeof Reflect&&Reflect.get?Reflect.get.bind():function(e,t,r){var p=_superPropBase(e,t);if(p){var o=Object.getOwnPropertyDescriptor(p,t);return o&&o.get?o.get.call(arguments.length<3?e:r):o?o.value:void 0}},_get.apply(this,arguments)}',


And here too

nicolo-ribaudo · 2024-05-17T18:45:22Z

CI failure is not related

nicolo-ribaudo · 2024-05-17T19:03:36Z

packages/babel-helpers/src/helpers-generated.ts

    defineEnumerableProperties: helper(
      "7.0.0-beta.0",
-      'export default function _defineEnumerableProperties(e,r){for(var t in r){var n=r[t];n.configurable=n.enumerable=!0,"value"in n&&(n.writable=!0),Object.defineProperty(e,t,n)}if(Object.getOwnPropertySymbols)for(var a=Object.getOwnPropertySymbols(r),b=0;b<a.length;b++){var i=a[b];(n=r[i]).configurable=n.enumerable=!0,"value"in n&&(n.writable=!0),Object.defineProperty(e,i,n)}return e}',
+      'import toPropertyKey from"toPropertyKey";export default function _defineEnumerableProperties(e,r){for(var t in r){var o=r[t];o.configurable=o.enumerable=!0,"value"in o&&(o.writable=!0),Object.defineProperty(e,t,o)}if(Object.getOwnPropertySymbols)for(var n=Object.getOwnPropertySymbols(r),a=0;a<n.length;a++){var i=n[a];(o=r[i]).configurable=o.enumerable=!0,"value"in o&&(o.writable=!0),Object.defineProperty(e,toPropertyKey(i),o)}return e}',


toPropertyKey is unnecessary here too, similarly to objectSpread

SukkaW · 2024-05-17T19:49:08Z

packages/babel-helpers/src/helpers-generated.ts

-    'import superPropBase from"superPropBase";import defineProperty from"defineProperty";function set(e,r,t,o){return set="undefined"!=typeof Reflect&&Reflect.set?Reflect.set:function(e,r,t,o){var f,i=superPropBase(e,r);if(i){if((f=Object.getOwnPropertyDescriptor(i,r)).set)return f.set.call(o,t),!0;if(!f.writable)return!1}if(f=Object.getOwnPropertyDescriptor(o,r)){if(!f.writable)return!1;f.value=t,Object.defineProperty(o,r,f)}else defineProperty(o,r,t);return!0},set(e,r,t,o)}export default function _set(e,r,t,o,f){if(!set(e,r,t,o||e)&&f)throw new TypeError("failed to set property");return t}',
+    'import defineProperty from"defineProperty";import superPropBase from"superPropBase";function set(e,r,t,o){return set="undefined"!=typeof Reflect&&Reflect.set?Reflect.set:function(e,r,t,o){var f,i=superPropBase(e,r);if(i){if((f=Object.getOwnPropertyDescriptor(i,r)).set)return f.set.call(o,t),!0;if(!f.writable)return!1}if(f=Object.getOwnPropertyDescriptor(o,r)){if(!f.writable)return!1;f.value=t,Object.defineProperty(o,r,f)}else defineProperty(o,r,t);return!0},set(e,r,t,o)}export default function _set(e,r,t,o,f){if(!set(e,r,t,o||e)&&f)throw new TypeError("failed to set property");return t}',


Maybe we should switch the order back 😂 That causes the gzip size to increase by 2 bytes.

Note that it doesn't actually matter much, as imports are removed when the helpers are inlined :)

#16501 will stop considering those imports in the computed size

liuxingbaoyu · 2024-05-17T22:18:02Z

Thanks!

nicolo-ribaudo · 2024-05-19T13:42:07Z

Thanks again!

coelhucas force-pushed the migrate-reflection-helpers branch 4 times, most recently from f573af9 to 3e52aa9 Compare May 17, 2024 15:29

coelhucas commented May 17, 2024

View reviewed changes

JLHwung reviewed May 17, 2024

View reviewed changes

coelhucas added 16 commits May 17, 2024 13:08

Migrate \defineEnumerableProperties\

39ed194

Migrate \defineProperty\

af21161

Migrate \get\

ca39ef7

Migrate \getPrototypeOf\

4619c28

Migrate set

50432d2

Migrate setPrototypeOf

f08ba58

Migrate superPropBase

c5195d2

Migrate construct

5d6db4b

Regenerate helpers-generated

a515b38

Fix: defineProperty value should allow any

92c5c89

Fix: use correct type for defineEnumerableProperties

e7fcdeb

Post make build changes

fa67c30

Replace PropertyKey with explicit string & symbol union

4913846

Replace for of with for

148b260

Fix set implementation

c5239a0

Remove let usage

c359ad1

coelhucas force-pushed the migrate-reflection-helpers branch from e97615a to c359ad1 Compare May 17, 2024 16:08

Adjust files affected by these migrations

cebcc0c

nicolo-ribaudo reviewed May 17, 2024

View reviewed changes

SukkaW suggested changes May 17, 2024

View reviewed changes

SukkaW reviewed May 17, 2024

View reviewed changes

coelhucas force-pushed the migrate-reflection-helpers branch from d1b11ec to 80e5eb8 Compare May 17, 2024 17:26

coelhucas requested review from JLHwung, nicolo-ribaudo and SukkaW May 17, 2024 17:58

nicolo-ribaudo reviewed May 17, 2024

View reviewed changes

coelhucas added 2 commits May 17, 2024 15:05

Fix setPrototypeOf

7bc742b

Update get helper

199472d

coelhucas force-pushed the migrate-reflection-helpers branch from a66b1ae to 199472d Compare May 17, 2024 18:11

fix remove additional bytes from get implementation

83f51d4

coelhucas requested a review from nicolo-ribaudo May 17, 2024 18:20

coelhucas marked this pull request as ready for review May 17, 2024 18:27

coelhucas changed the title ~~Migrate reflection helpers~~ Migrate reflection helpers to TypeScript May 17, 2024

nicolo-ribaudo reviewed May 17, 2024

View reviewed changes

Fix defineEnumerableProperties redundancy

30bc3d6

coelhucas requested a review from nicolo-ribaudo May 17, 2024 19:10

SukkaW reviewed May 17, 2024

View reviewed changes

Revert import order change for set helper

1182d42

coelhucas requested a review from SukkaW May 17, 2024 19:59

liuxingbaoyu approved these changes May 17, 2024

View reviewed changes

nicolo-ribaudo changed the title ~~Migrate reflection helpers to TypeScript~~ [helpers TS conversion] reflection May 18, 2024

nicolo-ribaudo added 5 commits May 18, 2024 11:46

Fix defineEnumerableProperties types

49e4633

Suggestion from Sukka

032842b

get changes

b505af0

fixup

4c5c474

Regenerate helpers

e46b6f7

nicolo-ribaudo force-pushed the migrate-reflection-helpers branch from a1edaad to e46b6f7 Compare May 18, 2024 12:06

Regenerate helpers

4be46e1

nicolo-ribaudo merged commit d6c2343 into babel:main May 19, 2024
51 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[helpers TS conversion] reflection #16507

[helpers TS conversion] reflection #16507

coelhucas commented May 17, 2024

babel-bot commented May 17, 2024 •

edited

coelhucas May 17, 2024

SukkaW May 17, 2024

coelhucas May 17, 2024

coelhucas May 17, 2024

SukkaW May 17, 2024

JLHwung May 17, 2024

nicolo-ribaudo May 17, 2024

nicolo-ribaudo May 17, 2024 •

edited

SukkaW May 17, 2024

nicolo-ribaudo May 17, 2024

SukkaW May 17, 2024 •

edited

SukkaW May 17, 2024

SukkaW May 17, 2024 •

edited

SukkaW May 17, 2024 •

edited

SukkaW May 17, 2024 •

edited

SukkaW May 17, 2024

nicolo-ribaudo May 17, 2024

nicolo-ribaudo commented May 17, 2024

nicolo-ribaudo May 17, 2024

SukkaW May 17, 2024

nicolo-ribaudo May 17, 2024

liuxingbaoyu commented May 17, 2024

nicolo-ribaudo commented May 19, 2024

		'import superPropBase from"superPropBase";import defineProperty from"defineProperty";function set(e,r,t,o){return set="undefined"!=typeof Reflect&&Reflect.set?Reflect.set:function(e,r,t,o){var f,i=superPropBase(e,r);if(i){if((f=Object.getOwnPropertyDescriptor(i,r)).set)return f.set.call(o,t),!0;if(!f.writable)return!1}if(f=Object.getOwnPropertyDescriptor(o,r)){if(!f.writable)return!1;f.value=t,Object.defineProperty(o,r,f)}else defineProperty(o,r,t);return!0},set(e,r,t,o)}export default function _set(e,r,t,o,f){if(!set(e,r,t,o\|\|e)&&f)throw new TypeError("failed to set property");return t}',
		'import defineProperty from"defineProperty";import superPropBase from"superPropBase";function set(e,r,t,o){return set="undefined"!=typeof Reflect&&Reflect.set?Reflect.set:function(e,r,t,o){var f,i=superPropBase(e,r);if(i){if((f=Object.getOwnPropertyDescriptor(i,r)).set)return f.set.call(o,t),!0;if(!f.writable)return!1}if(f=Object.getOwnPropertyDescriptor(o,r)){if(!f.writable)return!1;f.value=t,Object.defineProperty(o,r,f)}else defineProperty(o,r,t);return!0},set(e,r,t,o)}export default function _set(e,r,t,o,f){if(!set(e,r,t,o\|\|e)&&f)throw new TypeError("failed to set property");return t}',

[helpers TS conversion] reflection #16507

[helpers TS conversion] reflection #16507

Conversation

coelhucas commented May 17, 2024

babel-bot commented May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolo-ribaudo May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SukkaW May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SukkaW May 17, 2024 • edited

Choose a reason for hiding this comment

SukkaW May 17, 2024 • edited

Choose a reason for hiding this comment

SukkaW May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolo-ribaudo commented May 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liuxingbaoyu commented May 17, 2024

nicolo-ribaudo commented May 19, 2024

babel-bot commented May 17, 2024 •

edited

nicolo-ribaudo May 17, 2024 •

edited

SukkaW May 17, 2024 •

edited

SukkaW May 17, 2024 •

edited

SukkaW May 17, 2024 •

edited

SukkaW May 17, 2024 •

edited