Merge runtime plugin into babel-plugin-fbtee

[alexandernanberg]

Opening up this PR mainly for a discussion if we should merge the runtime plugin into the main plugin. There are callouts in the fbt docs about it here https://facebook.github.io/fbt/docs/getting_started_on_web

The approach I've taken here is likely not optimal, and is working more or less like before.

But I'm thinking if we should remove the intermediate step with the SENTINEL altogether and return the runtime format directly. It would remove the possibility of running the runtime transform separately, but I don't see a case where that is useful outside of Meta?

[cpojer]

This is great!

But I'm thinking if we should remove the intermediate step with the SENTINEL altogether and return the runtime format directly. It would remove the possibility of running the runtime transform separately, but I don't see a case where that is useful outside of Meta?

I agree. Personally, I don't remember why it was separate at Facebook (@kayhadrin do you?). Getting rid of the "sentinel" stuff and JSON embedding will make fbtee much cleaner, and will allow for improving the transform without having to care about this intermediate step. It could technically be slightly slower during collection since it would do more work (collection works by running Babel transforms and discarding the transform results, so it's a bit wasteful) – but I believe there is a lot of performance work to be done that will make the Babel plugin fast enough in other ways.

However, let's focus on merging the plugins first, and getting rid of the intermediate transformation in a separate PR.

[alexandernanberg]

The intermediate transform is conceptually gone if we merge the plugins (from the user pov), e.g. all test fixtures will have to updated to the runtime format.

Before

exports[`fbt variable binding detection should handle commonJS require() 1`] = `
const fbt = require("fbtee");
fbt._(
  /* __FBT__ start */ {
    jsfbt: { m: [], t: { desc: "Bar", text: "Foo" } },
    project: "",
  } /* __FBT__ end */
);

After

exports[`fbt variable binding detection should handle commonJS require() 1`] = `
const fbt = require("fbtee");
fbt._("Foo", null, {
  hk: "3ktBJ2",
});

What do you think about replacing the usage of assertSourceAstEqual and testCase test helpers with .toMatchSnapshot()?

[cpojer]

The intermediate transform is conceptually gone if we merge the plugins (from the user pov), e.g. all test fixtures will have to updated to the runtime format.

Yep, the point is that we should get rid of the double-transform internally so we don't have to encode the data, and then parse a string to parse JSON to transform it again.

I'm actually surprised merging them works like this, since the StringLiteral nodes you are matching don't exist when the transform runs. It's awesome that babel visits new nodes as well.

What do you think about replacing the usage of assertSourceAstEqual and testCase test helpers with .toMatchSnapshot()?

In favor of any improvements to make testing better, simpler and "jest native". I am not a fan of any of the confusing test utils and have cleaned up what I can so far.

[cpojer]

@alexandernanberg Hey, are you thinking of getting back to this one?

[alexandernanberg]

@cpojer Won't have time to look at this anytime soon unfortunately!