Repository navigation
Consider keeping class properties in loose mode #4263
Description
I was against #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.
Activity
gaearon commentedon Apr 5, 2018
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 commentedon Apr 5, 2018
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 commentedon Apr 5, 2018
That's too big of a difference between prod and dev environment—likely to cause bugs.
Timer commentedon Apr 5, 2018
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-envsupports it by default (it may already):https://github.com/facebook/create-react-app/blob/06176e1ea4de222039164e19b7f1546a20c6060f/packages/babel-preset-react-app/index.js#L95
hzoo commentedon Apr 5, 2018
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 commentedon Apr 5, 2018
👍 I'm cool with this.
Enable loose mode for `class-properties` (react#4248)