Skip to content

[Fresh] Initial Babel plugin implementation#15711

Merged
gaearon merged 7 commits intofacebook:masterfrom
gaearon:fresh-babel
May 22, 2019
Merged

[Fresh] Initial Babel plugin implementation#15711
gaearon merged 7 commits intofacebook:masterfrom
gaearon:fresh-babel

Conversation

@gaearon
Copy link
Collaborator

@gaearon gaearon commented May 22, 2019

This adds a basic implementation of the Babel plugin. It's incomplete and will be expanded as we go.

Its takes code like

function Foo() {
  // ...
}

let Bar = () => {
  // ...
}

and turns it into something like

function Foo() {
  // ...
}
_c = Foo;

let Bar = _c2 = () => {
  // ...
}

var _c;
var _c2;

__register__(_c, 'Foo');
__register__(_c2, 'Bar');

We rely on var and function declaration hoisting here. The registration all happens at the bottom so that if module init throws, we throw away the broken components.

The transform handles some basic cases like top-level functions and arrows (normal or exported). In the future I'll add support for some HOC patterns like memo. The transform intentionally injects variable in the middle of assignment so that in theory we could even do things like _c3 = forwardRef(_c2 = memo(_c1 = function() { })) without changing code semantics.

In the follow-ups, I'll handle more cases and tie this with our integration tests so we can verify the whole thing works as expected.

@sizebot
Copy link

sizebot commented May 22, 2019

Warnings
⚠️

Could not find build artifacts for base commit: 1c0bdb7

Generated by 🚫 dangerJS

@gaearon
Copy link
Collaborator Author

gaearon commented May 22, 2019

I initially thought component IDs would include the file name (e.g. __register__(Foo, 'Foo.js$Foo'). However I don't think this is actually a good idea. There is no guarantee that Babel receives correct file names. File name could be missing in babel.transform call for your integration — or worse, it could be the same for multiple files (e.g. a fake stub).

Instead I think the filename should be handled at the module runtime level. Whether we're in Metro/Webpack/something else, we should be able to know the module ID of currently initializing module inside the integration glue code. This is guaranteed to be correct because it's what the bundler actually uses. We'll also need this module ID anyway for preserving context identity. So this doesn't even add a new requirement.

Filenames are also problematic because they would mess up centralized development JS code caching if absolute. So it seems like Babel plugin shouldn't try to put the filenames into the output.

One caveat is that a bundler may unify several previously distinct modules into one. Like if you enable Rollup-like optimizations in development. In that case we'd need to know filenames or module IDs at the Babel call time. However it's probably a bad idea for a development / hot reloaded setup anyway (e.g. Rollup doesn't support it — rollup/rollup#50). So I don't think we need to handle this until somebody actually asks for it.

I've decided for now that the plugin doesn't need filename, and it will be handled by module runtime integration instead.
@bvaughn
Copy link
Contributor

bvaughn commented May 22, 2019

We rely on var and function declaration hoisting here. The registration all happens at the bottom so that if module init throws, we throw away the broken components.

Is it important that the vars themselves are also declared at the bottom and rely on hoisting? Seems like this should only be necessary fro the registration?

(Still reviewing... just an initial question.)

@gaearon
Copy link
Collaborator Author

gaearon commented May 22, 2019

Not strictly necessary. But since at least some need to be at the bottom (the ones in expressions like const Foo = _c = ...) I figured let's put all of them here. In general it would be good if we can move as much as possible down so that this transform isn't too noisy. Otherwise it can be a pain to debug when you don't have sourcemaps.

Edit: to be clear, for const Foo = _c = ... I could declare _c separately above, but I prefer to push these things down so that you normally don't look at them even when debugging compiled output.

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of just skimmed the Babel plug-in (since I don't write or look at these often) but I read the tests and snapshots more closely and they look good 👍


var _c3;

__register__(_c, 'Hello');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder why it sometimes uses single quotes and other times uses double quotes with escape slashes...

}).code;
}

describe('ReactFreshBabelPlugin', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice tests.

`);

function isComponentishName(name) {
return typeof name === 'string' && name[0] >= 'A' && name[0] <= 'Z';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😆

@gaearon
Copy link
Collaborator Author

gaearon commented May 22, 2019

Pushed a few bugfixes. I coalesced all variable definitions into one so that it doesn't take as many lines. Also checked that our own test suite works with the plugin enabled.

@gaearon gaearon merged commit 5c2124f into facebook:master May 22, 2019
@bvaughn
Copy link
Contributor

bvaughn commented May 22, 2019

Not strictly necessary. But since at least some need to be at the bottom...

Gotcha! That's what I assumed. I was just curious. 😄Thanks for confirming.

I coalesced all variable definitions into one so that it doesn't take as many lines.

Sounds like a nice tweak.

gaearon added a commit to gaearon/react that referenced this pull request Jun 11, 2019
* Add initial Babel plugin implementation

* Register exported functions

* Fix missing declarations

Always declare them at the bottom and rely on hoisting.

* Remove unused code

* Don't pass filename to tests

I've decided for now that the plugin doesn't need filename, and it will be handled by module runtime integration instead.

* Fix bugs

* Coalesce variable declarations
gaearon added a commit to gaearon/react that referenced this pull request Jun 19, 2019
* Add initial Babel plugin implementation

* Register exported functions

* Fix missing declarations

Always declare them at the bottom and rely on hoisting.

* Remove unused code

* Don't pass filename to tests

I've decided for now that the plugin doesn't need filename, and it will be handled by module runtime integration instead.

* Fix bugs

* Coalesce variable declarations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants