add support for logical assignments with private properties #11702

ryzokuken · 2020-06-10T15:35:16Z

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

Patches the logic for handling assignment operators and adds support for
handling the logical assignment operators appropriately.

codesandbox-ci · 2020-06-10T15:38:04Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 25bfbf4:

Sandbox	Source
babel-repl-custom-plugin	Configuration
babel-plugin-multi-config	Configuration

babel-bot · 2020-06-10T15:38:48Z

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

hzoo

awesome work, separating out logical/binary expression and a test case. I guess the others must be tested already? (||= &&=)

ryzokuken · 2020-06-10T16:28:55Z

@hzoo right, I'll add more tests for those.

ryzokuken · 2020-06-10T16:35:49Z

@hzoo PTAL now, I suppose this should suffice. Let me know if it needs any more work.

packages/babel-helper-member-expression-to-functions/src/index.js

nicolo-ribaudo · 2020-06-10T19:31:10Z

We already support private properties and logical assignment, just not together. Would it be ok for you to mark this as a "Bug fix" (since it's just an integration problem) and merge it in a patch release?

ryzokuken · 2020-06-11T05:19:16Z

We already support private properties and logical assignment, just not together. Would it be ok for you to mark this as a "Bug fix" (since it's just an integration problem) and merge it in a patch release?

Right, I was unsure, thanks for letting me know.

ryzokuken · 2020-06-11T05:31:13Z

Thanks @nicolo-ribaudo @JLHwung and others, I fixed the code and the PR body. Hope this is better.

packages/babel-helper-member-expression-to-functions/src/index.js

...es/babel-plugin-proposal-class-properties/test/fixtures/private/logical-assignment/output.js

Replace a hardcoded check for logical assignment operators with the LOGICAL_OPERATORS constant in plugin-proposal-logical-assignment-operators. Refs: babel#11702 (comment)

JLHwung

Thanks!

...es/babel-plugin-proposal-class-properties/test/fixtures/private/logical-assignment/output.js

Justin's concern is valid.

JLHwung · 2020-06-11T19:47:04Z

...ges/babel-plugin-proposal-class-properties/test/fixtures/private/logical-assignment/input.js

@@ -0,0 +1,11 @@
+class Foo {


Please also duplicate the test to test/fixtures/private-loose.

JLHwung · 2020-06-25T17:42:09Z

Friendly ping @ryzokuken :)

ryzokuken · 2020-06-25T17:43:11Z

Sorry @JLHwung, I'll finish it up this weekend.

ryzokuken · 2020-06-28T21:55:45Z

@jridgewell I pushed the test you asked for, but I still don't quite understand how to fix the memoization issue. Could you please elaborate what needs to be done in that regard?

JLHwung · 2020-06-29T19:45:03Z

...plugin-proposal-class-properties/test/fixtures/private-loose/logical-assignment/options.json

@@ -0,0 +1,6 @@
+{
+  "plugins": [
+    "proposal-logical-assignment-operators",


Can you add loose: true to both plugins?

{ "plugins": [ ["proposal-logical-assignment-operators", { "loose": true }], ["proposal-class-properties", { "loose": true }] ] }

The private-loose folder is test cases about loose options so we are sure that both loose and non-loose (default) are working.

JLHwung · 2020-06-29T20:40:04Z

@ryzokuken The memoization is done in

babel/packages/babel-helper-member-expression-to-functions/src/index.js

Lines 285 to 288 in 1dd94e8

    
           // Give the state handler a chance to memoise the member, since we'll 
        
           // reference it twice. The second access (the set) should do the memo 
        
           // assignment. 
        
           this.memoise(member, 2);

The second argument 2 means when we should memoize the member call. i.e. obj.self().#a += 1 is roughly transformed to

// obj.self().#a = obj.self().#a + 1
classFieldSet($objSelf = obj.self(), a, classFieldGet($objSelf, a) + 1)

via

this.set(member, t.binaryExpression("+", this.get(member), value))

We should memoize obj.self() in classFieldSet because it is the first argument of classFieldSet and thus evaluated before classFieldGet. However since in Babel we have called this.get and this.set sequentially, 2 is a counter to indicate that we should memoize the member in the second call of [this.get, this.set] sequence. (Think about TTL in ICMP protocol)

However obj.self().#a ??= 1 is different here, it should be transformed as

// obj.self().#a ?? (obj.self().#a = 1)
classFieldGet($objSelf = obj.self(), a) ?? classFieldSet($objSelf, a, 1)

// current PR
classFieldGet($objSelf, a) ?? classFieldSet($objSelf = obj.self(), a, 1)

via

t.LogicalExpression("??", this.get(member), this.set(member))

Note that the call sequence is unchanged -- this.get, this.set. But we should memoize in classFieldGet because it is now evaluated before classFieldSet. To achieve that we can decrement the memoize counter for logical expressions.

// add some comment about why 1 is chosen for logical expressions
this.memoise(member, 1);

jridgewell · 2020-06-30T22:23:00Z

Thank you for explaining @JLHwung, that's extremely thorough!

JLHwung · 2020-07-15T13:22:32Z

@ryzokuken 🏓️ Logical Assignment is seeking advancement in July meeting, will be great if we can ship this bug fix after that.

ryzokuken · 2020-07-15T13:29:29Z

@JLHwung my bad, I'll get that done.

ryzokuken · 2020-07-20T18:52:35Z

Update: I've made the change, I'll push this in a bit, sorry for the delay. I got super busy with all sorts of weird things.

ryzokuken · 2020-07-20T19:53:37Z

@JLHwung does this look good enough? Let me know if it does and I'll autosquash and force push.

JLHwung · 2020-07-20T19:59:18Z

@ryzokuken Can you update the test case (config/fixture) w.r.t.
#11702 (review)
and
#11702 (comment) ?

jridgewell · 2020-07-20T20:17:49Z

...el-plugin-proposal-class-properties/test/fixtures/private-loose/logical-assignment/output.js

+    _classPrivateFieldGet(this, _nullish) ?? _classPrivateFieldSet(this, _nullish, 42);
+    _classPrivateFieldGet(this, _and) && _classPrivateFieldSet(this, _and, 0);
+    _classPrivateFieldGet(this, _or) || _classPrivateFieldSet(this, _or, 0);
+    _classPrivateFieldGet(_this$self, _nullish) ?? _classPrivateFieldSet(_this$self = this.self(), _nullish, 42);


This test still needs to be updated. I believe the transform code is correct, the test is just out of date now.

ryzokuken · 2020-07-22T09:51:52Z

@jridgewell @JLHwung the tests should be updated now. Sorry for the delay again, I noticed the stage advancement yesterday.

JLHwung · 2020-07-22T12:10:05Z

The e2e was fixed on upstream main. Can you rebase?

Replace a hardcoded check for logical assignment operators with the LOGICAL_OPERATORS constant in plugin-proposal-logical-assignment-operators. Refs: babel#11702 (comment)

Patches the logic for handling assignment operators and adds support for handling the logical assignment operators appropriately. Fixes: babel#11646

Replace a hardcoded check for logical assignment operators with the LOGICAL_OPERATORS constant in plugin-proposal-logical-assignment-operators. Refs: babel#11702 (comment)

JLHwung

Justin's concern has been addressed. @ryzokuken Thanks for all the efforts!

ryzokuken · 2020-07-30T18:32:31Z

Thanks for all the help on this, everyone!

hzoo approved these changes Jun 10, 2020

View reviewed changes

ryzokuken force-pushed the gh-11646 branch from 95fec86 to 15150b3 Compare June 10, 2020 16:34

existentialism added the PR: New Feature 🚀 A type of pull request used for our changelog categories label Jun 10, 2020

existentialism approved these changes Jun 10, 2020

View reviewed changes

existentialism added the Spec: Logical Assignment label Jun 10, 2020

JLHwung reviewed Jun 10, 2020

View reviewed changes

packages/babel-helper-member-expression-to-functions/src/index.js Outdated Show resolved Hide resolved

JLHwung reviewed Jun 10, 2020

View reviewed changes

packages/babel-helper-member-expression-to-functions/src/index.js Outdated Show resolved Hide resolved

ryzokuken force-pushed the gh-11646 branch from 15150b3 to f781b05 Compare June 11, 2020 05:29

jridgewell reviewed Jun 11, 2020

View reviewed changes

packages/babel-helper-member-expression-to-functions/src/index.js Show resolved Hide resolved

...es/babel-plugin-proposal-class-properties/test/fixtures/private/logical-assignment/output.js Outdated Show resolved Hide resolved

kaicataldo approved these changes Jun 11, 2020

View reviewed changes

JLHwung previously approved these changes Jun 11, 2020

View reviewed changes

JLHwung added PR: Bug Fix 🐛 A type of pull request used for our changelog categories and removed PR: New Feature 🚀 A type of pull request used for our changelog categories labels Jun 11, 2020

jridgewell requested changes Jun 11, 2020

View reviewed changes

...es/babel-plugin-proposal-class-properties/test/fixtures/private/logical-assignment/output.js Show resolved Hide resolved

JLHwung reviewed Jun 11, 2020

View reviewed changes

JLHwung changed the base branch from master to main June 19, 2020 12:41

JLHwung reviewed Jun 29, 2020

View reviewed changes

jridgewell reviewed Jul 20, 2020

View reviewed changes

ryzokuken force-pushed the gh-11646 branch from 3d35b78 to 86cfb91 Compare July 22, 2020 17:15

ryzokuken added 2 commits July 22, 2020 22:47

add support for logical assignments with private properties

9cf68b1

Patches the logic for handling assignment operators and adds support for handling the logical assignment operators appropriately. Fixes: babel#11646

replace hardcoded logical assignment operators with constant

25bfbf4

Replace a hardcoded check for logical assignment operators with the LOGICAL_OPERATORS constant in plugin-proposal-logical-assignment-operators. Refs: babel#11702 (comment)

ryzokuken force-pushed the gh-11646 branch from 86cfb91 to 25bfbf4 Compare July 22, 2020 17:17

JLHwung approved these changes Jul 30, 2020

View reviewed changes

JLHwung merged commit 2ac49ba into babel:main Jul 30, 2020

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 30, 2020

github-actions bot locked as resolved and limited conversation to collaborators Oct 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for logical assignments with private properties #11702

add support for logical assignments with private properties #11702

ryzokuken commented Jun 10, 2020 •

edited by gitpod-io bot

codesandbox-ci bot commented Jun 10, 2020 •

edited

babel-bot commented Jun 10, 2020 •

edited

hzoo left a comment

ryzokuken commented Jun 10, 2020

ryzokuken commented Jun 10, 2020

nicolo-ribaudo commented Jun 10, 2020

ryzokuken commented Jun 11, 2020

ryzokuken commented Jun 11, 2020

JLHwung left a comment

JLHwung Jun 11, 2020

JLHwung commented Jun 25, 2020

ryzokuken commented Jun 25, 2020

ryzokuken commented Jun 28, 2020

JLHwung Jun 29, 2020

JLHwung commented Jun 29, 2020

jridgewell commented Jun 30, 2020

JLHwung commented Jul 15, 2020

ryzokuken commented Jul 15, 2020

ryzokuken commented Jul 20, 2020

ryzokuken commented Jul 20, 2020

JLHwung commented Jul 20, 2020

jridgewell Jul 20, 2020

ryzokuken commented Jul 22, 2020

JLHwung commented Jul 22, 2020

JLHwung left a comment

ryzokuken commented Jul 30, 2020

add support for logical assignments with private properties #11702

add support for logical assignments with private properties #11702

Conversation

ryzokuken commented Jun 10, 2020 • edited by gitpod-io bot

codesandbox-ci bot commented Jun 10, 2020 • edited

babel-bot commented Jun 10, 2020 • edited

hzoo left a comment

Choose a reason for hiding this comment

ryzokuken commented Jun 10, 2020

ryzokuken commented Jun 10, 2020

nicolo-ribaudo commented Jun 10, 2020

ryzokuken commented Jun 11, 2020

ryzokuken commented Jun 11, 2020

JLHwung left a comment

Choose a reason for hiding this comment

JLHwung Jun 11, 2020

Choose a reason for hiding this comment

JLHwung commented Jun 25, 2020

ryzokuken commented Jun 25, 2020

ryzokuken commented Jun 28, 2020

JLHwung Jun 29, 2020

Choose a reason for hiding this comment

JLHwung commented Jun 29, 2020

jridgewell commented Jun 30, 2020

JLHwung commented Jul 15, 2020

ryzokuken commented Jul 15, 2020

ryzokuken commented Jul 20, 2020

ryzokuken commented Jul 20, 2020

JLHwung commented Jul 20, 2020

jridgewell Jul 20, 2020

Choose a reason for hiding this comment

ryzokuken commented Jul 22, 2020

JLHwung commented Jul 22, 2020

JLHwung left a comment

Choose a reason for hiding this comment

ryzokuken commented Jul 30, 2020

ryzokuken commented Jun 10, 2020 •

edited by gitpod-io bot

codesandbox-ci bot commented Jun 10, 2020 •

edited

babel-bot commented Jun 10, 2020 •

edited