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

JavaScript Object Destructuring with default values does not work after minification #394

Closed
failwyn opened this issue May 9, 2024 · 2 comments

Comments

@failwyn
Copy link

failwyn commented May 9, 2024

JavaScript object destructuring with default values does not work after minification, the original parameter name is not maintained, so the value is never extracted from the source object and always has the default value.

Code

const someFunc = ({ val, op = 'eq' }) => {
    var useOp = op + val;
};

var args = {
    val: 'va',
    op: 'contains'
};

someFunc(args);

Actual
const someFunc=({val:t,n="eq"})=>{var i=n+t};var args={val:"va",op:"contains"};someFunc(args)

Expected
const someFunc=({val:t,op:n="eq"})=>{var i=n+t};var args={val:"va",op:"contains"};someFunc(args)

My current fix of setting the Field property of the ObjectLiteralProperty in ParseObjectLiteralProperty when isBindingPattern is true and the current token is Assign causes the tests for Bugs 201 and 345 to fail... although, in both cases, I believe the new results are correct.

Bug 201:
Code

const sum = function ({ foo = 2, dummy = 3 }) {
	return foo + dummy;
};

const sum2 = ({ foo2 = 2, dummy2 = 3 }) => foo2 + dummy2;

Expected
const sum=function({t=2,n=3}){return t+n},sum2=({t=2,n=3})=>t+n

Actual
const sum=function({foo:t=2,dummy:n=3}){return t+n},sum2=({foo2:t=2,dummy2:n=3})=>t+n

Bug 345:
Code

'use strict';

function test1(
    {
        verylongname_x = 1,
        verylongname_y = 2
    } = {}) {
    console.log(verylongname_x + verylongname_y);
}

function test2(verylongname_x = 1, verylongname_y = 2) {
    console.log(verylongname_x + verylongname_y);
}

Expected
"use strict";function test1({n=1,t=2}={}){console.log(n+t)}function test2(n=1,t=2){console.log(n+t)}

Actual
"use strict";function test1({verylongname_x:n=1,verylongname_y:t=2}={}){console.log(n+t)}function test2(n=1,t=2){console.log(n+t)}

If you agree with the results, I have some questions so that I can generate the ObjectLiteralField in the intended way instead of the way I'm doing it now, which just doesn't feel right.

In Bug 390 you mentioned that the code in that section gets confused with two different types of notations/types, would you be able to describe those issues when you get a chance?

@failwyn
Copy link
Author

failwyn commented May 9, 2024

The fix requires parsing the same token twice... It doesn't look like there is a way to back up the SourceContext or JSScanner... so I cloned them before ParseObjectPropertyValue, then if it's a binding pattern and the current token is Assign, I replace m_currentToken and m_scanner with the clones, and call ParseObjectPropertyValue... This doesn't feel right, but it feels better than creating the ObjectLiteralField directly using the value.Context...

field = new ObjectLiteralField(value.Context.Code, PrimitiveType.Other, value.Context.Clone());

JSParser line 4923

else if (nextToken == JSToken.Comma || nextToken == JSToken.RightCurly || nextToken == JSToken.Assign)
{
    isBindingPattern |= nextToken == JSToken.Assign;

    // just a name lookup; the property name is implicit
    ParsedVersion = ScriptVersion.EcmaScript6;
    var fieldContext = m_currentToken.Clone();
    var fieldScanner = m_scanner.Clone();
    value = ParseObjectPropertyValue(isBindingPattern);

    if (isBindingPattern && m_currentToken.Is(JSToken.Assign))
    {
        // we need to back up to properly parse the field name...
        if (m_settings.LocalRenaming != LocalRenaming.KeepAll)
        {
            m_currentToken = fieldContext;
            m_scanner = fieldScanner;
            field = ParseObjectLiteralFieldName();
        }

        var assignContext = m_currentToken.Clone();
        GetNextToken();
        value = new InitializerNode(assignContext.Clone()) {
            Binding = value,
            AssignContext = assignContext,
            Initializer = ParseExpression(true)
        };
    }
}

@trullock
Copy link
Owner

Sounds sensible enough

failwyn pushed a commit to failwyn/NUglify that referenced this issue May 10, 2024
…pt object destructuring with default values and property renaming (trullock#394)
trullock added a commit that referenced this issue Jun 11, 2024
Fixes issue with default values and javascript destructuring (#394)
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

2 participants