Create-react-app: Consider keeping class properties in loose mode

[gaearon]

I was against https://github.com/facebook/create-react-app/pull/4248 but thought a bit more about it and now I’m not sure.

We already deviate from the spec. That’s not great, but that’s an existing problem. We can choose to address it now or we can choose to address it later.

React apps rely on class properties for defining methods and state. However Object.defineProperty is known to be relatively expensive (at least it used to be). That doesn’t mean browser’s native implementation would necessarily be expensive (I don’t think anyone measured). But that switching from existing output to new output will likely introduce performance regressions.

How severe will this regression be? I don’t know. But we need to further explore this tradeoff. In particular we might want to measure the regression on a large React project that relies a lot on class properties. If it’s significant we might want to enable loose behavior, and postpone changing it to be spec-compliant until we have some plan for addressing this.

[gaearon]

Another data point: at FB we changed it to use loose because it caused a significant bundle size regression after converting to Babel 7.

[Timer]

What if we only applied loose mode in production?

If we remain strict in development, users can develop against spec so that when they target "latest chrome version" and get native class property support, things work as expected.

Making it loose in production will prevent bundle size from increasing and not introduce potential performance regressions until this is fleshed out further in the coming months.

[gaearon]

That's too big of a difference between prod and dev environment—likely to cause bugs.

[Timer]

I think we're introducing the potential for bugs in either direction -- if we go this route I think we should force the class properties transform to be on, no matter target browser support (I think we do this now).

We should add a comment to this explaining not to remove it once preset-env supports it by default (it may already):
https://github.com/facebook/create-react-app/blob/06176e1ea4de222039164e19b7f1546a20c6060f/packages/babel-preset-react-app/index.js#L95

[hzoo]

Yeah 👍 to changing. I was hesitant to make it default but overall that seems like the best decision as a compiler even if it means "config" for users. I guess it's what people expect already, hard to decide

[gaearon]

if we go this route I think we should force the class properties transform to be on, no matter target browser support (I think we do this now)

👍 I'm cool with this.