perf(gatsby/babel): Optimize hook destructuring

[blainekasten]

Description

Thanks to @developit for giving us a great rundown of the benefit of this feature and also shipping it to next.

I took the liberty to make it into a package so other ecosystems and companies outside of Gatsby/next could benefit from it. Is there a problem with this approach?

Documentation

Do we document our babel setup anywhere? or micro optimizations we do? cc @gatsbyjs/learning

Related Issues

#19943

[pvdz]

um, ok :)

[laurieontech]

@blainekasten We have section on Gatsby internals that it could fit well in! We recently updated the webpack config with @wardpeet's new chunking functionality.

https://www.gatsbyjs.org/docs/production-app/

[pieh]

Do we need this as separate package or can it be just a file inside our gatsby-preset-gatsby? As soon as we release this is separate package we defacto become maintainers for this? (If there is no standalone package like this already).

[blainekasten]

So I'm not certain we need this at all...

I looked at the output from next when you build and have a useState:

It compiles to this:

var a=Object(Q.useState)(0),b=a[0],c=a[1];

Which is peculiar, because it's different than the test that is written in general. But then I tested my changes with gatsby and I got the same results. Seems great, but then I tested against gatsby master and got the same results

So I'm not sure if this plugin in general is just not doing what it's supposed to do for us or next, or the way it's working means we don't actually need this plugin for some reason.

cc @developit for advice because I'm not really sure what the aim still is. Do we want to see {0:a,1:b} or b=a[0],c=a[1] ?

[developit]

This only affects modern output where destructuring is preserved, or legacy output when Babel is in non-loose mode (example). Babel's loose mode output already dissolves destructured arrays (example) and treats them as objects.

It's safe to apply this optimization for both modern and non-modern output, but it will only provide value in modern output.

[developit]

Just to clarify, here's a breakdown:

Input:

const [count, setCount] = useState(0);

Output: (non-modern / ES5)

(note that this is identical to the output without hook destructuring optimization)

var _temp = Object(Q.useState)(0),
    count = _temp[0],
    setCount = _temp[1];

Output: (modern)

var { 0: count, 1: setCount } = Object(Q.useState)(0);

[blainekasten]

@developit Thanks for the clarification :) I was talking to @wardpeet shortly before you posted this and realized exactly what you described. Good news, I tested this in a gatsby project by creating a .babelrc with this config:

{
  "presets": [
    [
      "@babel/preset-env",
      {
        "targets": { "esmodules": true }
      }
    ],
    "@babel/preset-react"
  ],
  "plugins": [
    "../gatsby/packages/babel-preset-gatsby/optimize-hook-destructuring.js"
  ]
}

and saw the expected object destructured from the plugin.

So I would say this plugin is ready to be merged, even though the value it gives won't be seen until we ship modern builds (soon!)

[blainekasten]

@laurieontech I think I'm going to skip adding to docs for this one. This is a very small benefit that users don't really need to think about or know about. modern builds will be a more applicable effort to document. Let me know if you disagree 👍

[pvdz]

/home/circleci/project/packages/babel-preset-gatsby/src/optimize-hook-destructuring.ts
11:6 error Use an interface instead of a type @typescript-eslint/consistent-type-definitions

[laurieontech]

@blainekasten I think that's ok given the changes.

[pvdz]

Errors for the build are unrelated to this. Let's merge it.

[kitos]

@blainekasten @pieh @developit I am not completely sure whether it is babel issue or my/gatsby misconfiguration. But after updating gatsby this optimization broke my build, because of the following syntax:

let [{ someProp, ...rest }, setState] = useState()

I assumed that this is a babel plugin issue (even though I wasn't able to reproduce it in babel repl).
You can check find a PR with potential fix here.

[pieh]

---edited

Ignore this comment

@kitos

I just tried yours and what our current babel transform generate is (check snapshot):

it(`should object destructuring`, () => {
    const input = trim`
      import { useState } from 'react';
      let [{ someProp, ...rest }, setState] = useState({ someProp: 'a', otherProp: 'b' });
    `

    expect(() => babel(input)).not.toThrow()
    expect(babel(input)).toMatchInlineSnapshot(
      `"\\"use strict\\";var _react=require(\\"react\\");let{0:{someProp,...rest},1:setState}=(0,_react.useState)({someProp:'a',otherProp:'b'});"`
    )
  })

So theoretically it should be fine? What's exact error you are seeing? Is it actually babel or syntax error or runtime error? Also do you actually use the code as is - are you not passing any initial state value? I think it would be undefined if not specified so you would get "Cannot read property 'someProp' of undefined" in runtime if you are using it exactly as you show in example?

Also as sidenote - we diverged a bit from initial implementation in this PR already because of #23438 (but this doesn't seem to affect your example)

[pieh]

Ah, ok I just checked your PR in babel - yeah - seems like it you accurately found real culprit and seems like your PR already getting approvals, so it's bit of our hands (other than maybe trying to temprorily disable this transform while we wait for new release for babel-plugin-proposal-object-rest-spread

Sorry for sending previous comment before checking on the linked PR (please ignore :P)