[ GENERAL ][ BUGFIX ][ jest/preprocessor.js ] - Invalid use of destructuring

[umairda]

Test Plan:

I had the same issues running tests as mentioned in: #19859

I followed the suggestions but ran into another error:

  ● Test suite failed to run

    /Users/umair/projects/my-project/node_modules/react-native/jest/preprocessor.js:49
              {sourceType: 'script', ...nodeOptions, ast: false},
                                     ^^^

    SyntaxError: Unexpected token ...

The preprocessor.js code in question is:

      // node specific transforms only
      return babelTransformSync(
        src,
        Object.assign(
          {filename: file},
          {sourceType: 'script', ...nodeOptions, ast: false},
        ),
      ).code;

Specifically line 46: {sourceType: 'script', ...nodeOptions, ast: false}, is an invalid use of object destructuring and since this object is part of an Object.assign statement I simply broke the constituents up into arguments as follows:

        Object.assign(
          {filename: file},
          {sourceType: 'script'},
          nodeOptions, 
          {ast: false}
        ),

Release Notes:

[GENERAL][BUGFIX][jest/preprocessor.js] - fixes invalid use of object destructuring in preprocessor.js

[elicwhite]

What version of node are you running? My understanding is that the ... there is reasonable.

[umairda]

v8.0.0

It looks like I have to pass the --harmony flag when running node to enable object destructuring

But the code in general is strange. If destructuring is being used then why is Object.assign also used? It seems like you could write the second argument simply as:

{
   filename:file,
   sourceType:'script',
   ...nodeOptions,
   ast:false
}

[elicwhite]

It looks like node rest/spread syntax started working without the harmony flag in 8.3.0. This should work if you update to that.

@hramos, we should make sure our minimum supported version is set to 8.3.0.

————
I agree, this function call is strange and is unnecessarily using Object.assign and spread. If you change this PR to just be the spread version, we would merge this in.

[umairda]

You are right. I updated to node v8.3.0 and it works.

I went ahead and updated the pr so that portion of the code uses destructuring only and got rid of the Object.assign.

I re-ran all my tests and they work fine whereas before they were complaining about the use of the spread operator.

[janicduplessis]

Maybe we should update https://github.com/facebook/react-native/blob/master/local-cli/server/checkNodeVersion.js#L17 to 8.3 then. There might be some other mentions in the doc too.

[facebook-github-bot]

@TheSavior is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[hramos]

#20236

[elicwhite]

It looks like the commit didn't auto close this PR: 9d5bd50