ionic-team / ionic-framework

A powerful cross-platform UI toolkit for building native-quality iOS, Android, and Progressive Web Apps with HTML, CSS, and JavaScript.
https://ionicframework.com
MIT License
50.48k stars 13.53k forks source link

bug: scroll element not reachable in CI #29419

Closed stewones closed 2 weeks ago

stewones commented 2 weeks ago

Prerequisites

Ionic Framework Version

v8.x

Current Behavior

currently if we try to access the content instance in the constructor (ie: before ionic initializes) it breaks with TypeError: Cannot read properties of undefined (reading 'scrollToBottom')

this error happens because content is null at that point

Expected Behavior

to not break cause this messes up with CI, even though it might be developer's fault trying to access it before initialization.

Steps to Reproduce

https://github.com/stewones/ionic-content-scroll-issue

  1. run the app with nx serve app
  2. test the app with nx test app

Code Reproduction URL

https://github.com/stewones/ionic-content-scroll-issue

Ionic Info

   Ionic CLI : 7.2.0

Utility:

   cordova-res : not installed globally
   native-run  : not installed globally

System:

   NodeJS : v18.16.0
   npm    : 9.5.1
   OS     : macOS Unknown

Additional Information

a workaround would be checking for the scroll element existence from the client, but IMO this should be baked into the core.

if (await this.content.getScrollElement()) {
  this.content.scrollToBottom();
}
sean-perkins commented 2 weeks ago

Hello @stewones thanks for opening this issue!

After reviewing your reproduction case and issue, I do not believe this is a concern of Ionic Framework. Accessing an ElementRef in Angular before the view has rendered will always produce undefined.

In your reproduction you should be checking that content is defined before accessing the method on it.

this.content?.scrollToBottom();

In your test spec, due to the conditions of your app, you would need to mock the state of the class that you are testing for:

test('increment and scroll to bottom', async () => {
  const mockScrollToBottom = jest.fn();
  instance.content = {
    scrollToBottom: mockScrollToBottom
  } as any;

  instance.increment();
  fixture.detectChanges();

  expect(instance.value()).toBe(421);
  expect(mockScrollToBottom).toHaveBeenCalled();
});

I believe with these considerations we can close: https://github.com/ionic-team/ionic-framework/pull/29410.

Let me know if you have any questions!