Skip to content

Commit 54fa5bf

Browse files
awmackcopybara-github
awmack
authored and
copybara-github
committed
fix!: resolve issue #305 where looping could happen in typical declarative implementations
Removes a failure mode possible in React and others when handling error events. A fetch error from `<gmpx-place-overview>`, when caught, can result in the framework re-assigning the `place` property, triggering an infinite re-render loop. This change restores diff checking to the `place` property and implements regression tests. BREAKING CHANGE: If you rely on setting `elem.place = elem.place` to force a refresh, you will need to switch to a new, explicit `.refresh()` method. PiperOrigin-RevId: 972823989
1 parent e01ed52 commit 54fa5bf

7 files changed

Lines changed: 336 additions & 58 deletions

File tree

‎src/place_building_blocks/place_data_consumer_test.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -146,14 +146,14 @@ describe('Place Data Consumer base class', () => {
146146
expect(placeChangedSpy).toHaveBeenCalledWith(FAKE_CONTEXT_PLACE, undefined);
147147
});
148148

149-
it('invokes callback when place from context is updated', async () => {
149+
it('invokes callback when place from context is refreshed', async () => {
150150
const consumer =
151151
await preparePlaceConsumer(/* place= */ undefined, FAKE_CONTEXT_PLACE);
152152
const provider = consumer.parentElement as PlaceDataProvider;
153153
const placeChangedSpy =
154154
spyOn(TestPlaceDataConsumerConcrete.prototype, 'placeChangedCallback');
155155

156-
provider.place = FAKE_CONTEXT_PLACE;
156+
await provider.refresh();
157157
await env.waitForStability();
158158

159159
expect(consumer.getPlace()).toBe(FAKE_CONTEXT_PLACE);
@@ -184,7 +184,7 @@ describe('Place Data Consumer base class', () => {
184184
const placeChangedSpy = spyOn(
185185
TestPlaceDataConsumerConcrete.prototype, 'placeChangedCallback');
186186

187-
provider.place = FAKE_CONTEXT_PLACE;
187+
await provider.refresh();
188188
await env.waitForStability();
189189

190190
expect(consumer.getPlace()).toBe(FAKE_PLACE);

‎src/place_building_blocks/place_data_provider/README.md‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,11 @@ import { PlaceDataProvider } from '@googlemaps/extended-component-library/place_
2828

2929
## Attributes and properties
3030

31-
| Attribute | Property | Property type | Description | Default | [Reflects?](https://open-wc.org/guides/knowledge/attributes-and-properties/#attribute-and-property-reflection) |
32-
| --------------------- | ------------------- | ----------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------- | -------------------------------------------------------------------------------------------------------------- |
33-
| `auto-fetch-disabled` | `autoFetchDisabled` | `boolean` | If `place` is provided with a `Place` or `PlaceResult` instance, but does not contain fields required by child components, this element will make a request to the Place API to retrieve the missing data. Set `auto-fetch-disabled` to prevent the component from performing these requests. | `false` | ✅ |
34-
| `fields` | `fields` | `string[] \| undefined` | Manually specify the fields to request from the Places API.<br/><br/>If unspecified, the component will request only fields used by child components. | | ✅ |
35-
| `place` | `place` | `string\|Place\|PlaceResult \| undefined` | The place to be displayed by this component. Provide a [Place ID](https://developers.google.com/maps/documentation/places/web-service/place-id?utm_source=github&utm_medium=documentation&utm_campaign=&utm_content=web_components) as a string to have the component look up and display details from the Place API. The component will not make further API requests if child components are added at a later time. If required, explicitly request a data fetch by re-setting `place` to the same Place ID as before.<br/><br/>Alternatively, assign a `Place` or `PlaceResult` object to the `place` property to render it directly (note that the attribute, on the other hand, only accepts a Place ID string). | | ❌ |
31+
| Attribute | Property | Property type | Description | Default | [Reflects?](https://open-wc.org/guides/knowledge/attributes-and-properties/#attribute-and-property-reflection) |
32+
| --------------------- | ------------------- | ----------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------- | -------------------------------------------------------------------------------------------------------------- |
33+
| `auto-fetch-disabled` | `autoFetchDisabled` | `boolean` | If `place` is provided with a `Place` or `PlaceResult` instance, but does not contain fields required by child components, this element will make a request to the Place API to retrieve the missing data. Set `auto-fetch-disabled` to prevent the component from performing these requests. | `false` | ✅ |
34+
| `fields` | `fields` | `string[] \| undefined` | Manually specify the fields to request from the Places API.<br/><br/>If unspecified, the component will request only fields used by child components. | | ✅ |
35+
| `place` | `place` | `string\|Place\|PlaceResult \| undefined` | The place to be displayed by this component. Provide a [Place ID](https://developers.google.com/maps/documentation/places/web-service/place-id?utm_source=github&utm_medium=documentation&utm_campaign=&utm_content=web_components) as a string to have the component look up and display details from the Place API. The component will not make further API requests if child components are added at a later time. If required, explicitly request a data fetch by calling `refresh()`.<br/><br/>Alternatively, assign a `Place` or `PlaceResult` object to the `place` property to render it directly (note that the attribute, on the other hand, only accepts a Place ID string). | | ❌ |
3636

3737
## Slots
3838

@@ -44,6 +44,14 @@ This component uses [named slots](https://developer.mozilla.org/en-US/docs/Web/A
4444
| initial-loading | If specified, display this content when the component is initially loading Places data. Content in this slot will receive Places data, but some or all fields may be undefined. |
4545
| error | If specified, display this content when there was any error loading data from the Places API. |
4646

47+
## Methods
48+
49+
### `refresh()`
50+
51+
Re-queries the set of consumed fields and re-fetches place data.
52+
53+
**Returns:** `Promise<void>`
54+
4755
## Events
4856

4957
| Name | React Prop | Type | Description |

‎src/place_building_blocks/place_data_provider/place_data_provider.ts‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -76,13 +76,13 @@ export class PlaceDataProvider extends BaseComponent {
7676
* as a string to have the component look up and display details from the
7777
* Place API. The component will not make further API requests if child
7878
* components are added at a later time. If required, explicitly request a
79-
* data fetch by re-setting `place` to the same Place ID as before.
79+
* data fetch by calling `refresh()`.
8080
*
8181
* Alternatively, assign a `Place` or `PlaceResult` object to the `place`
8282
* property to render it directly (note that the attribute, on the other hand,
8383
* only accepts a Place ID string).
8484
*/
85-
@property({type: String, hasChanged: () => true})
85+
@property({type: String})
8686
place?: string|Place|PlaceResult;
8787

8888
/**
@@ -117,14 +117,25 @@ export class PlaceDataProvider extends BaseComponent {
117117

118118
private set contextPlace(place: Place|undefined) {
119119
// Force an update to the consumer even if the place is the same object.
120-
// This allows developers to refresh the consumers by setting
121-
// provider.place = provider.place, for example if they added/fetched new
122-
// fields to the place object themselves.
120+
// This ensures consumers are notified when contextPlace is refreshed
121+
// via provider.refresh().
123122
this.placeContextProvider.setValue(place, /* force= */ true);
124123
}
125124

126125
private static readonly placeLookup = new CachedPlaceLookup(CACHE_SIZE);
127126

127+
/**
128+
* Re-queries the set of consumed fields and re-fetches place data.
129+
*/
130+
async refresh(): Promise<void> {
131+
try {
132+
await this.updatePlace();
133+
} catch (error: unknown) {
134+
this.handleError(error);
135+
throw error;
136+
}
137+
}
138+
128139
protected override render() {
129140
return choose(this.loadingState, [
130141
[LoadingState.EMPTY, () => html``],

‎src/place_building_blocks/place_data_provider/place_data_provider_test.ts‎

Lines changed: 130 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -66,11 +66,13 @@ describe('PlaceDataProvider', () => {
6666
// Properties of Place are getter-only in the typings
6767
Object.defineProperty(place, 'displayName', {
6868
get: () => 'Fake Place',
69+
configurable: true,
6970
});
7071
}
7172
if (fields.includes('rating')) {
7273
Object.defineProperty(place, 'rating', {
7374
get: () => 5,
75+
configurable: true,
7476
});
7577
}
7678
return {place};
@@ -238,29 +240,9 @@ describe('PlaceDataProvider', () => {
238240
expect(fetchFieldsSpy).toHaveBeenCalledOnceWith({fields: ['displayName']});
239241
});
240242

241-
it(`fetches new child's field when same place id is set again`, async () => {
242-
const {provider, fetchFieldsSpy} = await prepareState(html`
243-
<gmpx-place-data-provider place="id8-B">
244-
<gmpx-test-consumer field="displayName">
245-
</gmpx-test-consumer>
246-
</gmpx-place-data-provider>
247-
`);
248-
249-
fetchFieldsSpy.calls.reset();
250-
const consumer = new TestConsumer();
251-
consumer.field = 'rating';
252-
provider.appendChild(consumer);
253-
provider.place = provider.place;
254-
await env.waitForStability();
255-
256-
expect(fetchFieldsSpy).toHaveBeenCalledOnceWith({
257-
fields: ['displayName', 'attributions', 'rating'],
258-
});
259-
});
260-
261-
it(`fetches new child's field when same place obj is set again`, async () => {
243+
it(`does not re-fetch when same place object is set again`, async () => {
262244
const fetchFieldsSpy = jasmine.createSpy('fetchFields');
263-
const place = makeFakePlace({id: 'id8-C'});
245+
const place = makeFakePlace({id: 'id8-D'});
264246
attachFetchFieldsSpy(place, fetchFieldsSpy);
265247

266248
const {provider} = await prepareState(html`
@@ -271,39 +253,43 @@ describe('PlaceDataProvider', () => {
271253
`);
272254

273255
fetchFieldsSpy.calls.reset();
274-
const consumer = new TestConsumer();
275-
consumer.field = 'rating';
276-
provider.appendChild(consumer);
277256
provider.place = provider.place;
278257
await env.waitForStability();
279258

280-
expect(fetchFieldsSpy).toHaveBeenCalledOnceWith({
281-
fields: ['displayName', 'attributions', 'rating'],
282-
});
259+
expect(fetchFieldsSpy).not.toHaveBeenCalled();
283260
});
284261

285-
it(`updates a consumer when the same place object is set again`, async () => {
286-
const place = makeFakePlace({id: 'id8-D'});
262+
// Issue #305: Prevents infinite error loops when a requesterror handler re-renders with the same Place instance.
263+
it('does not enter an error loop when gmpx-requesterror listener re-assigns identical place', async () => {
264+
let callCount = 0;
265+
const quotaError = new Error(
266+
'PLACES_GET_PLACE: 429 RESOURCE_EXHAUSTED: Quota exceeded');
267+
const place = makeFakePlace({
268+
id: 'test-data-provider-quota',
269+
fetchFields: () => {
270+
callCount++;
271+
return Promise.reject(quotaError);
272+
},
273+
});
287274

288-
// Use auto-fetch-disabled so the fetch callback doesn't update the
289-
// consumer. We're testing that setting the place property will trigger an
290-
// update on its own.
291-
const {provider} = await prepareState(html`
292-
<gmpx-place-data-provider .place=${place} auto-fetch-disabled>
293-
<gmpx-test-consumer field="displayName">
294-
</gmpx-test-consumer>
275+
const root = env.render(html`
276+
<gmpx-place-data-provider .place=${place}>
277+
<gmpx-test-consumer field="displayName"></gmpx-test-consumer>
295278
</gmpx-place-data-provider>
296279
`);
280+
const provider =
281+
root.querySelector<PlaceDataProvider>('gmpx-place-data-provider')!;
282+
283+
let listenerCalls = 0;
284+
provider.addEventListener('gmpx-requesterror', () => {
285+
listenerCalls++;
286+
provider.place = place;
287+
});
297288

298-
const consumer = new TestConsumer();
299-
consumer.field = 'rating';
300-
provider.appendChild(consumer);
301-
await env.waitForStability();
302-
consumer.resetUpdateCount();
303-
provider.place = provider.place;
304289
await env.waitForStability();
305290

306-
expect(consumer.getUpdateCount()).toEqual(1);
291+
expect(callCount).toBe(1);
292+
expect(listenerCalls).toBe(1);
307293
});
308294

309295
it('fetches when the place is changed', async () => {
@@ -532,4 +518,104 @@ describe('PlaceDataProvider', () => {
532518
const consumer = provider.children[0] as TestConsumer;
533519
expect(consumer.contextPlace?.displayName).toBe('Foo Inc');
534520
});
521+
522+
describe('refresh', () => {
523+
it(`fetches new child's field when refresh is called with place id`, async () => {
524+
const {provider, fetchFieldsSpy} = await prepareState(html`
525+
<gmpx-place-data-provider place="id8-B">
526+
<gmpx-test-consumer field="displayName">
527+
</gmpx-test-consumer>
528+
</gmpx-place-data-provider>
529+
`);
530+
531+
fetchFieldsSpy.calls.reset();
532+
const consumer = new TestConsumer();
533+
consumer.field = 'rating';
534+
provider.appendChild(consumer);
535+
await provider.refresh();
536+
await env.waitForStability();
537+
538+
expect(fetchFieldsSpy).toHaveBeenCalledOnceWith({
539+
fields: ['displayName', 'attributions', 'rating'],
540+
});
541+
});
542+
543+
it(`fetches new child's field when refresh is called with place obj`, async () => {
544+
const fetchFieldsSpy = jasmine.createSpy('fetchFields');
545+
const place = makeFakePlace({id: 'id8-C'});
546+
attachFetchFieldsSpy(place, fetchFieldsSpy);
547+
548+
const {provider} = await prepareState(html`
549+
<gmpx-place-data-provider .place=${place}>
550+
<gmpx-test-consumer field="displayName">
551+
</gmpx-test-consumer>
552+
</gmpx-place-data-provider>
553+
`);
554+
555+
fetchFieldsSpy.calls.reset();
556+
const consumer = new TestConsumer();
557+
consumer.field = 'rating';
558+
provider.appendChild(consumer);
559+
await provider.refresh();
560+
await env.waitForStability();
561+
562+
expect(fetchFieldsSpy).toHaveBeenCalledOnceWith({
563+
fields: ['displayName', 'attributions', 'rating'],
564+
});
565+
});
566+
567+
it('re-fetches fields on demand when refresh is called with Place object', async () => {
568+
const fetchFieldsSpy = jasmine.createSpy('fetchFields').and.resolveTo({});
569+
const place = makeFakePlace({
570+
id: 'test-data-provider-refresh',
571+
fetchFields: fetchFieldsSpy,
572+
});
573+
574+
const root = env.render(html`
575+
<gmpx-place-data-provider .place=${place}>
576+
<gmpx-test-consumer field="displayName"></gmpx-test-consumer>
577+
</gmpx-place-data-provider>
578+
`);
579+
await env.waitForStability();
580+
expect(fetchFieldsSpy).toHaveBeenCalledTimes(1);
581+
582+
const provider =
583+
root.querySelector<PlaceDataProvider>('gmpx-place-data-provider')!;
584+
await provider.refresh();
585+
await env.waitForStability();
586+
expect(fetchFieldsSpy).toHaveBeenCalledTimes(2);
587+
});
588+
589+
it('sets error state, dispatches event, and rejects when fetch fails', async () => {
590+
const fetchFieldsSpy = jasmine.createSpy('fetchFields').and.rejectWith(
591+
new Error('Quota exceeded'));
592+
const place = makeFakePlace({
593+
id: 'test-data-provider-refresh-error',
594+
fetchFields: fetchFieldsSpy,
595+
});
596+
597+
const requestErrorListener = jasmine.createSpy('requestErrorListener');
598+
const root = env.render(html`
599+
<gmpx-place-data-provider
600+
.place=${place}
601+
@gmpx-requesterror=${(e: Event) => requestErrorListener(e)}
602+
>
603+
<gmpx-test-consumer field="displayName"></gmpx-test-consumer>
604+
<div slot="error">Custom Error Content</div>
605+
</gmpx-place-data-provider>
606+
`);
607+
await env.waitForStability();
608+
expect(fetchFieldsSpy).toHaveBeenCalledTimes(1);
609+
expect(requestErrorListener).toHaveBeenCalledTimes(1);
610+
611+
const provider =
612+
root.querySelector<PlaceDataProvider>('gmpx-place-data-provider')!;
613+
614+
await expectAsync(provider.refresh()).toBeRejectedWithError('Quota exceeded');
615+
await env.waitForStability();
616+
expect(fetchFieldsSpy).toHaveBeenCalledTimes(2);
617+
expect(requestErrorListener).toHaveBeenCalledTimes(2);
618+
expect(provider.shadowRoot?.querySelector('slot[name="error"]')).not.toBeNull();
619+
});
620+
});
535621
});

0 commit comments

Comments
 (0)