Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make runtime label extraction opt-in #2762

Closed
srmagura opened this issue May 21, 2022 · 8 comments · Fixed by #2815
Closed

Make runtime label extraction opt-in #2762

srmagura opened this issue May 21, 2022 · 8 comments · Fixed by #2815
Assignees
Labels

Comments

@srmagura
Copy link
Contributor

It's a known issue that you can get hydration warnings in Safari when using runtime label extraction and SSR. Safari implements Proper Tail Calls, while Node does not. This results in Safari getting the wrong label since the component name is nowhere to be found in the stack trace.

#2569 was a fundamentally flawed attempt to suppress these hydration warnings. I do not believe it is possible to suppress only the hydration warnings caused by Proper Tail Calls without also suppressing tons of legitimate hydration warnings.

Proposal

  • Add a enableRuntimeLabelExtractionInDev flag to createCache. The flag would be an optional boolean, with true as the default value. true should be the default because runtime label extraction is a useful feature for the majority of users.
  • Document that runtime label extraction can cause hydration warnings in Safari, and suggest using @emotion/babel-plugin, setting enableRuntimeLabelExtractionInDev to false, or using a different browser if it is bothering you.

The new flag would also be beneficial for the people who believe runtime label extraction is slowing down their app in development. I believe this issue is described in #2303. The "new" label extraction implementation that I wrote is faster than the old one, but it could still cause a noticeable slowdown if the number of elements is very large.

Looking for approval on this from @Andarist and/or @mitchellhamilton, then I can code it.

@Andarist
Copy link
Member

Follow-uping on the comment here - I would flip the proposal: I would make this label extraction opt-in (yes, it will be less discoverable and thus less useful, we should document it though). To simplify things we can use a global flag instead of adding stuff to the cache.

@srmagura srmagura changed the title Make it possible to disable runtime label extraction since it causes SSR hydration warnings in Safari Make runtime label extraction opt-in Jul 3, 2022
@srmagura srmagura assigned srmagura and unassigned Andarist Jul 3, 2022
@srmagura
Copy link
Contributor Author

srmagura commented Jul 3, 2022

Sounds good. Can we make this change without releasing a new major version? It shouldn't break anyone's code (assuming they are not misusing those labels...), but it is a big change to the developer experience.

For implementing it, I am thinking it would be a new module in @emotion/react that looks like this:

// internal, not exported by index.js
export let enableRuntimeLabelExtractionInDev = false

export function enableRuntimeLabelExtractionInDev() {
    enableRuntimeLabelExtractionInDev = true
}

Does that look right to you?

@Andarist
Copy link
Member

Andarist commented Jul 4, 2022

Yes, I think it can be released within the current major version because this extraction was always dev-only so people shouldn't rely on this anyhow.

For implementing it, I am thinking it would be a new module in @emotion/react that looks like this:

Actually, I was thinking about globalThis.EMOTION_RUNTIME_AUTO_LABEL (or something like that). The reason to prefer this is that in certain scenarios it could spare people from adding a direct dependency on @emotion/react. We need to remember that people are often using Emotion indirectly through other packages. Your idea here is good too, and more conventional, but I think that perhaps this time circumstances justify a global flag.

@emmatown
Copy link
Member

emmatown commented Jul 4, 2022

While I don't feel super strongly about it, tbh I think if it's not enabled by default, it should just be removed. If you have to opt-in to getting labels, you might as well opt-in to the better approach (using the Babel plugin).

@Andarist
Copy link
Member

Andarist commented Jul 5, 2022

In general, I agree with this - but there is a raise in the popularity of alternative transpilers and nowadays some people might not want to include Babel in their toolchain just to get those labels. Of course, without Babel, they lose on some other stuff too but those other ones we can't as easily provide using runtime shenanigans

@emmatown
Copy link
Member

emmatown commented Jul 5, 2022

Yeah, I guess I'm just thinking that when would someone actually want to enable it? Saying "hey, you can have these labels if set this global and you'll sometimes get these hydration mismatch errors and those errors are sometimes correct and sometimes not" is a such weird story to tell in docs.

@Andarist
Copy link
Member

Andarist commented Jul 5, 2022

Yeah, it's true - OTOH this works most of the time. The main situation in which a mismatch can happen right now is if you are SSRing and using Safari as your browser. I would be okay with removing it altogether too - I'm just worried that some people might find this to deteriorate their DX. With the global flag, we'll at least have a way for them to opt into this (we don't really have to document this flag at the beginning) and it will allow us to monitor how many issues we get about it without putting too much pressure on us.

@emmatown
Copy link
Member

emmatown commented Jul 5, 2022

With the global flag, we'll at least have a way for them to opt into this (we don't really have to document this flag at the beginning) and it will allow us to monitor how many issues we get about it without putting too much pressure on us.

Sure, that's sounds good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants