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

fix: revert use useSyncExternalStore to subscribe for context updates #1755

Merged
merged 1 commit into from
Aug 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions packages/react/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,12 @@
},
"dependencies": {
"@babel/runtime": "^7.20.13",
"@lingui/core": "4.4.1",
"use-sync-external-store": "^1.2.0"
"@lingui/core": "4.4.1"
},
"devDependencies": {
"@lingui/jest-mocks": "*",
"@testing-library/react": "^14.0.0",
"@types/react": "^18.2.13",
"@types/use-sync-external-store": "^0.0.3",
"eslint-plugin-react": "^7.32.2",
"eslint-plugin-react-hooks": "^4.6.0",
"react": "^18.2.0",
Expand Down
79 changes: 47 additions & 32 deletions packages/react/src/I18nProvider.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,53 @@ describe("I18nProvider", () => {
expect(container.textContent).toEqual("1_cs2_cs")
})

it(
"given 'en' locale, if activate('cs') call happens before i18n.on-change subscription in useEffect(), " +
"I18nProvider detects that it's stale and re-renders with the 'cs' locale value",
() => {
const i18n = setupI18n({
locale: "en",
messages: { en: {} },
})
let renderCount = 0

const CurrentLocaleContextConsumer = () => {
const { i18n } = useLingui()
renderCount++
return <span data-testid="child">{i18n.locale}</span>
}

/**
* Note that we're doing exactly what the description says:
* but to simulate the equivalent situation, we pass our own mock subscriber
* to i18n.on("change", ...) and in it we call i18n.activate("cs") ourselves
* so that the condition in useEffect() is met and the component re-renders
* */
const mockSubscriber = jest.fn(() => {
i18n.load("cs", {})
i18n.activate("cs")
return () => {
// unsubscriber - noop to make TS happy
}
})
jest.spyOn(i18n, "on").mockImplementation(mockSubscriber)

const { getByTestId } = render(
<I18nProvider i18n={i18n}>
<CurrentLocaleContextConsumer />
</I18nProvider>
)

expect(mockSubscriber).toHaveBeenCalledWith(
"change",
expect.any(Function)
)

expect(getByTestId("child").textContent).toBe("cs")
expect(renderCount).toBe(2)
}
)

it("should render children", () => {
const i18n = setupI18n({
locale: "en",
Expand Down Expand Up @@ -179,36 +226,4 @@ describe("I18nProvider", () => {

expect(getByText("Ahoj světe")).toBeTruthy()
})

it("when re-rendered with new i18n instance, it will forward it to children", () => {
const i18nCs = setupI18n({
locale: "cs",
messages: { cs: {} },
})

const i18nEn = setupI18n({
locale: "en",
messages: { en: {} },
})

const CurrentLocaleContextConsumer = () => {
const { i18n } = useLingui()
return <span data-testid="dynamic">{i18n.locale}</span>
}

const { container, rerender } = render(
<I18nProvider i18n={i18nCs}>
<CurrentLocaleContextConsumer />
</I18nProvider>
)

expect(container.textContent).toEqual("cs")

rerender(
<I18nProvider i18n={i18nEn}>
<CurrentLocaleContextConsumer />
</I18nProvider>
)
expect(container.textContent).toEqual("en")
})
})
65 changes: 24 additions & 41 deletions packages/react/src/I18nProvider.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,4 @@
import React, {
ComponentType,
FunctionComponent,
useCallback,
useRef,
} from "react"
import { useSyncExternalStore } from "use-sync-external-store/shim"

import React, { ComponentType, FunctionComponent } from "react"
import type { I18n } from "@lingui/core"
import type { TransRenderProps } from "./TransNoContext"

Expand Down Expand Up @@ -38,6 +31,7 @@ export const I18nProvider: FunctionComponent<I18nProviderProps> = ({
defaultComponent,
children,
}) => {
const latestKnownLocale = React.useRef<string | undefined>(i18n.locale)
/**
* We can't pass `i18n` object directly through context, because even when locale
* or messages are changed, i18n object is still the same. Context provider compares
Expand All @@ -49,51 +43,42 @@ export const I18nProvider: FunctionComponent<I18nProviderProps> = ({
*
* We can't use useMemo hook either, because we want to recalculate value manually.
*/
const makeContext = useCallback(
const makeContext = React.useCallback(
() => ({
i18n,
defaultComponent,
_: i18n.t.bind(i18n),
}),
[i18n, defaultComponent]
)
const context = useRef<I18nContext>(makeContext())

const subscribe = useCallback(
(onStoreChange: () => void) => {
const renderWithFreshContext = () => {
context.current = makeContext()
onStoreChange()
}
const propsChanged =
context.current.i18n !== i18n ||
context.current.defaultComponent !== defaultComponent
if (propsChanged) {
renderWithFreshContext()
}
return i18n.on("change", renderWithFreshContext)
},
[makeContext, i18n, defaultComponent]
)

const getSnapshot = useCallback(() => {
return context.current
}, [])
const [context, setContext] = React.useState<I18nContext>(makeContext())

/**
* Subscribe for locale/message changes
*
* the I18n object passed via props is the single source of truth for all i18n related
* I18n object from `@lingui/core` is the single source of truth for all i18n related
* data (active locale, catalogs). When new messages are loaded or locale is changed
* we need to trigger re-rendering of LinguiContext consumers.
* we need to trigger re-rendering of LinguiContext.Consumers.
*/
const contextObject = useSyncExternalStore(
subscribe,
getSnapshot,
getSnapshot
)
React.useEffect(() => {
const updateContext = () => {
latestKnownLocale.current = i18n.locale
setContext(makeContext())
}
const unsubscribe = i18n.on("change", updateContext)

/**
* unlikely, but if the locale changes before the onChange listener
* was added, we need to trigger a rerender
* */
if (latestKnownLocale.current !== i18n.locale) {
updateContext()
}
return unsubscribe
}, [i18n, makeContext])

if (!contextObject.i18n.locale) {
if (!latestKnownLocale.current) {
process.env.NODE_ENV === "development" &&
console.log(
"I18nProvider rendered `null`. A call to `i18n.activate` needs to happen in order for translations to be activated and for the I18nProvider to render." +
Expand All @@ -103,8 +88,6 @@ export const I18nProvider: FunctionComponent<I18nProviderProps> = ({
}

return (
<LinguiContext.Provider value={contextObject}>
{children}
</LinguiContext.Provider>
<LinguiContext.Provider value={context}>{children}</LinguiContext.Provider>
)
}
18 changes: 0 additions & 18 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3123,13 +3123,11 @@ __metadata:
"@lingui/jest-mocks": "*"
"@testing-library/react": ^14.0.0
"@types/react": ^18.2.13
"@types/use-sync-external-store": ^0.0.3
eslint-plugin-react: ^7.32.2
eslint-plugin-react-hooks: ^4.6.0
react: ^18.2.0
react-dom: ^18.2.0
unbuild: ^1.1.2
use-sync-external-store: ^1.2.0
peerDependencies:
react: ^16.8.0 || ^17.0.0 || ^18.0.0
languageName: unknown
Expand Down Expand Up @@ -4397,13 +4395,6 @@ __metadata:
languageName: node
linkType: hard

"@types/use-sync-external-store@npm:^0.0.3":
version: 0.0.3
resolution: "@types/use-sync-external-store@npm:0.0.3"
checksum: 161ddb8eec5dbe7279ac971531217e9af6b99f7783213566d2b502e2e2378ea19cf5e5ea4595039d730aa79d3d35c6567d48599f69773a02ffcff1776ec2a44e
languageName: node
linkType: hard

"@types/yargs-parser@npm:*":
version: 21.0.0
resolution: "@types/yargs-parser@npm:21.0.0"
Expand Down Expand Up @@ -14833,15 +14824,6 @@ __metadata:
languageName: node
linkType: hard

"use-sync-external-store@npm:^1.2.0":
version: 1.2.0
resolution: "use-sync-external-store@npm:1.2.0"
peerDependencies:
react: ^16.8.0 || ^17.0.0 || ^18.0.0
checksum: 5c639e0f8da3521d605f59ce5be9e094ca772bd44a4ce7322b055a6f58eeed8dda3c94cabd90c7a41fb6fa852210092008afe48f7038792fd47501f33299116a
languageName: node
linkType: hard

"util-deprecate@npm:^1.0.1, util-deprecate@npm:~1.0.1":
version: 1.0.2
resolution: "util-deprecate@npm:1.0.2"
Expand Down
Loading